Quick action context menu for WTrackProperty #2612
Conversation
|
Wow this is a big diff! WTrackTableView is down to a much more manageable 920 lines of code. |
There was a problem hiding this comment.
I did a quick test run and everything seems to be working as intended. Sometimes the right-click on the track text has no effect, but I wasn't able to discover a repeatable pattern.
The new WTrackMenu is still a monster class with some flaws. Hopefully we are able to fix this later, maybe by extraction the actual tasks out of the menu and moving them into their sub-modules with a better separation of concerns. I already started to do this for the custom tags feature, which might serve as a blueprint when finished.
Good job, Harshit! Thank you for pursuing this issue and trying to incorporate all our proposed changes.
|
@Be-ing LGTM. At least I don't expect any severe, unintended side-effects. |
|
Yay! Thank you for finally taking care of this. I have been saying for a while this would be a good little introductory project for a new contributor but no one volunteered to do it. So thanks for finally stepping up to do it. This is a big UX improvement and the code is significantly better organized too. |
|
Thanks for your prompt advice and reviews as well. I actually got to learn a lot more about code organisation and C++/Qt. |
|
Great this feature is finally implemented!! |
|
Don't forget DlgTagFetcher too. |
|
From what I can tell from my first trials, all the styles I need to add will also allow to style the Preferences window. Ouuuweeh! A lot of work and testing required...but in the end we may have the entire Mixxx GUI themed per skin. |
|
Unfortunately I found a major design flaw that I didn't spot during the review. The function WTrackMenu::getTrackPointers() now loads all selected tracks into memory at once before processing them. Formerly we iterated over the TrackModel rows and loaded each single track just before processing it. After processing it was dropped (and probably saved) immediately. This needs to be fixed before releasing 2.3. |
|
Storing a potentially huge list of track pointers in memory is a bad idea. |
|
I'm unclear what the problem is. WTrackMenu::getTrackPointers dynamically builds the list of TrackPointers from the TrackModel as needed; they are not stored past that. |
|
They are now loaded upfront instead of lazily during the iteration as before. It is only for a short time, but this will kill Mixxx for large track selections which are already an open issue. I also found another performance hog when tracks are loaded temporarily just for collecting the model indices. I suggest to first merge this PR before the next round of fixes. Fixing the main issue will take longer, because it requires a major rework. Maybe we can adopt the approach introduced in the Custom Tags branch. But this is still requires a lot of work. |
Can we use generators? |
|
Coroutines are a C++20 feature. Please elaborate if you have some ideas. Just mentioning buzzwords doesn't get us anywhere ;) |
|
@hacksdump Please don't get me wrong. It's absolutely necessary to explore the possibilities. But all those fancy features doesn't help us if we have no idea about how to leverage them. The classical, polymorphic approach I applied in #2656 is cumbersome and requires some amount of boilerplate. Unfortunately I have no idea how to reduce this with our current set of tools, namely C++17. Any ideas? |
I missed the fact that we are currently using C++17. I checked #2656 and the implementation seems promising. Thanks for pointing out the memory hogging behavior of I will move any further discussion to relevant PR discussion. |
|
@hacksdump Thank you very much for your first contribution. |
|
Signed the form. |
|
Done! |
|
I'm excited to try this out. Thank you! |
These were broken in mixxxdj#2612.
Fixes https://bugs.launchpad.net/mixxx/+bug/1277689
Now WTrackTableView Context menu actions are also accessible from WTrackProperty widget in the player deck.
For brevity, only selected actions have been added.