Collection panel: Append to Current adds wrong tracks

[Johannes Sasongko]

Steps:
1. Select an item in the Collection panel.
2. Right click on a different item, Add to Current.

Expected: tracks from 2nd item added.
Actual: tracks from 1st item added.

Console message:
** (exaile.py:4790): CRITICAL **: pygtk_generic_tree_model_get_value: assertion `VALID_ITER(iter, tree_model)' failed

Confirmed in trunk. Probably not in 0.3.2 (tree model change is only in trunk), but need to test.

[Johannes Sasongko (sjohannes)]

> Confirmed in trunk. Probably not in 0.3.2 (tree model change is only in trunk), but need to test.

Oh wait, that's talking about the wrong tree model. The Collection panel model is unchanged IIRC.

[Dustin Spicuzza (dustin-virtualroadside)]

This was quite a tricky bug. The problem is that the default handler for button_press on the treeview hasn't run yet, so when the menu is shown, the rows that the user intends to select haven't actually been selected yet. The way the menu is designed, the selected rows shouldn't be evaluated until one of the menu items has been pressed -- BUT the rating widget needs to know what it should show the rating for, so it evaluates it as soon as the menu is shown.

To fix this, just delay generating the menu items. I found that you cannot just simply do idle_add to the menu.popup() function as this has an unpleasant visual effect due to the tree view quickly focusing/unfocusing. My solution hides the menu as soon as it is shown (while suppressing the show/hide events), does idle_add for the menu regeneration function, and then once the function is run it shows the menu again and re-enables the show/hide events.

I've attached the patch before committing this because I'm not quite sure if this is the right way to go about fixing this. Suggestions/thoughts welcome -- if nobody comments, then I'll just push this, as it doesn't seem to have any adverse effects except a *very* small delay in showing the menu, but I imagine most users won't notice that.

[Mathias Brodala (mathbr)]

Possible fix with r3976, gotta watch out for possible side effects. In the end this change makes sure the menu is shown upon button release, not press. Thus the selection can be properly updated before showing the menu.

[Dustin Spicuzza (dustin-virtualroadside)]

Yeah, there was a side effect. In the collection panel, when you select an item, then shift-right click on another item, I expect all of the items to be selected. Instead, only the item you right clicked on is selected. In fact, if you select all of the items, and then right click, then it just selects a single item, making it impossible to select multiple items.

Showing the menu on button release is rather non-intuitive to me. Additionally, your fix would mean that to get proper behavior, developers have to remember to only show the menu on button release -- which as I said, isn't intuitive.

What didn't you like about my patch? It solves the problem without any side effects (that I see so far). I'm still not sure it's the best way to solve it, but I do think it's a better way than moving the menu to button release.

[Mathias Brodala (mathbr)]

I made one adjustment with r3983 and the behavior seems fine now.

Your patch seems like a hack which I’d like to avoid. Heck I’m not even satisfied with my own solution ATM but I’d rather go along that line and possibly enforce proper selection instead of quickly hiding and showing menus. Something odd is bound to happen that way.

[Dustin Spicuzza (dustin-virtualroadside)]

It does work correctly now, but showing the menu on release behavior is less than desirable. I'll think on this, and see if there's a better way.

[Dustin Spicuzza (dustin-virtualroadside)]

I retract my complaint about showing the menu on release. I'm using my Windows at the moment, and *everything* shows the menu at release. I never even noticed.

We should probably change all of the right click events to be consistent then -- if one of them is on release, all of them should be. Which I think is mostly the case... except for right click on the playlist, I think.

[Mathias Brodala (mathbr)]

That’d be a Windows thing then, but the default GTK behavior is to show menus upon button press which allows you to keep the button pressed and activate items upon button release. Since I am using this very often, I’d like to have a proper fix for this issue.

[Dustin Spicuzza (dustin-virtualroadside)]

Well, keeping in mind that the problem is that the rows aren't selected by the default button handler until after the menu is generated, thoughts on other ways to fix this:

a) Figure out how to call the default handler before calling menu.popup()
b) Implement the default handler in exaile and call that before calling menu.popup()
c) There are variations on my patch that are possible and look less hacky:
   - Add an idle_add() to the menu.popup function (but this has a weird visual glitch)
   - Add an idle_add() to the regenerate_menu function (but this has a weird visual glitch)
d) My patch isn't that bad. Just adds a delay in rendering the menu.

[Johannes Sasongko (sjohannes)]

Just a note, I think this worked correctly on Exaile 0.2 with
DragTreeView, which fixed a lot of GTK+ treeview annoyances. If I remember
correctly, it works by manipulating the selection on button-press-event;
it also handles the corner cases with shift+click and ctrl+click correctly.

[Mathias Brodala (mathbr)]

I’ve just reverted my changes so that the menus appear on button press again like default GTK behavior. I don’t expect a fix for this to arrive on time for 3.3.0 or even 3.3.1 since the click handling actually needs to be overhauled. Making this work properly for the playlist view was possible since it does not use the DragTreeView.

[Dustin Spicuzza (dustin-virtualroadside)]

I don't think reverting was a good idea.

Sure it isn't the ideal solution, but at least it *works* and does a right thing, whereas without it the collection does the *wrong* thing -- and as such is rather useless if one uses it for drag and drop at all (which is probably a common thing).

I feel like all of the side panels need to be rewritten anyways, but we should keep them working as they get rewritten. I don't imagine a user is going to care *why* it works, just as long as it works.

[Mathias Brodala (mathbr)]

Temporary fix via button release re-applied with r4252. It should work until the panels are properly rewritten.