Add Hotcue Color Replace Dialog #2547
Conversation
|
Any comments? |
|
I would like to avoid this sort of wizard-style dialog box that does multiple different things at once and forces the user to make a bunch of choices that activate or deactivate different UI elements. Instead, I'd prefer to have one box for recoloring tracks, and another for recoloring hotcues. They can share code and components for efficiency, but they shouldn't be presented in the same UI. We can have two separate options to launch either one. One reason for this is I expect the UIs for these to diverge, since hotcues and tracks are extremely different items and we may want to provide more complex options for deciding, for instance, how to recolor tracks. ("set red if genre == house"). There's going to be essentially no overlap between the kinds of mutations people will want to do, except that they both involve assigning colors to a bunch of items in a database. That's too weak and technical a reason to combine the UIs. I can also imagine adding a button in the hotcue color setter so that users can discover the option more easily, so it would make sense to have a way to launch a UI that only works on hotcues. (For instance, a button in the cuecolor popup that says "bulk assign"). same for tracks. Since this PR doesn't support hotcue recoloring anyway, please have this PR just address recoloring tracks, and make the UI only for that. Again, you can write the code with an eye toward also working with recoloring hotcues, but that should be done in a separate class. |
|
Ok, this dialog only does track color changes now. |
Holzhaus
changed the title
|
|
|
thanks! Can you reduce the big blank area? |
|
Can we make it affect selected tracks only and put it the context menu of the tracks in the library or did we explicitly agree against this? |
That would be redundant, since you can already select multiple tracks, right click and do "Select Color -> Some color" in the library view. This dialog is for bulk changes that affect the whole DB. |
Done. |
Ah right, but then someone could also just sort by colors, select the entire range of the tracks with the color they want, and recolor it that way. |
|
To be honest, I actually started this for bulk replacing hotcue colors but couldn't really implement that because I'm waiting for #2520, but then thought "Hey, we could also use this for track colors", and later the placeholders for hotcue colors were removed ;-) You're right, I'm not even certain this dialog is actually helpful except for cases where you database is huge... |
|
It certainly makes sense for the hotcues ("we give the dialog some features the library option can't fulfill.") but not really for the track colors. |
Holzhaus
changed the title
|
in that case, are we sure we want this PR at all? The code might still be useful when we need to make the editor for hotcues, but if there isn't a strong need for it we shouldn't merge it. |
|
No, that'why I changed the title and moved it back to "in progress" on the project board. I'll use this for the hotcue color replace dialog after #2520 has been merged. |
Holzhaus
force-pushed
the
mass-color-replace
branch
2 times, most recently
from
bab0707
to
7926008
Compare
|
Here's how it looks right now:
|
|
I still have problems with invalidating the track cache, but apart from that it's already working. If you don't see the changed colors, try restarting Mixxx. |
|
Please be aware that write access to the database from another thread may cause locks for the main thread if it tries to make any changes concurrently, e.g. when saving track objects or adding/removing tracks to/from playlist or crates. The corresponding queries will be aborted, the changes are lost and a warning appears only in the log, unnoticed by the user. We already have known issues while a library scan is running and updating the database. Blocking the main thread like during many other actions is also bad. But at least it doesn't cause data loss. |
|
Ok, I think this ready to merge now. The only remaining item of discussion is if we drop @daschuer Can you elaborate what kind of inconsistencies you expect when using this? The only thing I can imagine that could happen is that the user manually edits the color of a cue that is going to be replaced after starting the DB update. In this case it's possible that the color is still being replaced. From the user perspective, this behaviour shouldn't be suprising. And changing a color would only be possible via a controller script because the dialog is application modal. |
|
If we use processEvents() from a slot, it is a unprotected recursive call, because all slots are called from processEvents(). I am unsure if this is already an issue here, but such a call hidden in a dialog can be a trap, if we use this pattern elsewhere and its very unexpected. Because of various issues we have removed all such processEvents() calls from the Mixxx source. Analyze all edge cases in this case is some work and you cannot be sure if you concider all current and future cases. From top of my head I think it is an issue to have interlaced sqlite transactions. |
I agree that the transaction argument is valid. Should we prevent that our local transaction gets hijacked by using a separate, cloned database connection? Then AutoDJ might fail to write its changes during the batch processing. Sending any signal in Qt could have a similar, cascading effect. Is processEvents() really so much different? Maybe those processEvents() calls just revealed other shortcomings in the code and were not the actual cause of the issues. |
|
Each thread has it's own main loop calling processEvents() in a loop checking if a signal arrives to calls the slot. If a slot emits a signal with a direct connected slot, it is processed almost like a direct callback. This cause a sort of predictable interlaceing, that's all. |
|
If we can ensure that the code calling processEvents() is reentrant regarding this yield point it should not be an issue. By calling processEvents() you are responsible for ensuring that any other of your slots might be invoked (by the same thread) until execution continues at that point. Let's check that this is the case for If transaction hijacking is considered a problem I would simply use a transaction for each track. It takes more time and we loose the rollback-all feature, but who cares. You could at least complete the operation simply by executing it again. |
|
Is the GUI stalling issue really such a problem? So let's go the save and easy route and remove the processEvent() call. |
This operation is supposed to be executed on a large number of tracks. Therefore we need a progress dialog. A modal progress dialog calls processEvents() behind the scenes. |
|
Ups, never mind. I think it was an merge issue. Now I build the clean branch. |
|
Ok, I did some tests. Interestingly unlike the normal preferences dialog Mixxx can be not controlled once the GUI dialog is open. So there is a low risk that the processEvents() does any harm. There is an issue with the checkboxes that makes the "All warning visible" if checkbox is checked and it can also be hidden if no check-boxes is are checked. The rest is OK. |
Sorry, I don't understand. The warning should be shown if no checkbox is checked and hidden otherwise. Doesn't this work for you? |
|
That dos not work reliable after you have play around with them for a while. |
Oh right, I forgot to connect the |
|
Merge? |
|
ping |
|
I cannot reproduce the checkbox bug @daschuer encountered. |
|
@ywwg: Is everything fine from your review? |
|
@ywwg has not raised any concerns about this in weeks, so I'm going to go ahead and merge it now. |
|
Whoops, Windows builds are failing: |
This is an inital implementation of the Replace Color Dialog. I don't want to rebase on #2520 all the time, so this is based on
master.Until #2520 has been merged, there'll be some drawbacks:No support for replacing hotcue colors yet, just track colorsUses plainQColorDialoginstead ofWColorPicker, so no support for "no track color" yetCan be found via the global library menu (will be moved to a button on the color settings preferences page after Make Hotcue RGB colors configurable #2530 has been merged)Screenshot:
