Optimize Interface preferences pane #527
Conversation
…changes the setting.
… of clicks to change the settings.
…o change the setting - Fixed bug: key lock preference was not changing to the previously-stored value but resetting to default every time
…rotection" preference options to check boxes, reducing the number of clicks to set each to one. - Consolidated spacing of the track load options to one row - Removed erroneous documentation on the behavior of the "reset speed and pitch" control (0=off apparently, which makes more sense) - Set "reset speed and pitch" default to 'off' (it was ambiguous what the previous default was due to the erroneous documentation)
|
Hi @Pegasus-RPG Thank you for the Pull request. First of all: Here my comments driven from my taste, looking only on the screenshots:
|
|
I talked to @Pegasus-RPG about this PR, and while I think it's a major improvement, we really need to have better tests for the preference system before we go monkeying around. So I've asked him to please write tests for whatever controls he wants to change before he changes them. (including first-run, load and save, pref -> ui and ui -> pref interaction). Once we have a good battery of tests, then we can come back to this PR and know that everything still works. |
|
Regarding tests, instantiating a DlgPrefControls requires many other objects (MixxxMainWindow, SkinLoader, PlayerManager, ConfigObject,) necessitating a good grasp of the testing infrastructure which I don't yet have. (How many of these can be mocked for example?) |
|
thanks for working on this. I have come around and agree that requiring tests for the prefs pane is very difficult and relies on a lot of subtle interaction. So I think the next best thing is coming up with a short list of things to check for to confirm that a prefs panel works:
I have definitely seen changes that break one or more of these items, especially the exit/restart flow. If you can sanity check a few of the items, we can merge this in and have other people hammer on it. I also think it's ok to fix prefs one pane at a time, even if that means we branch 2.1 with only some panes fixed |
|
@daschuer One of my goals with this is to reduce the size of the pane so the window can fit completely on smaller screens so the Apply/Cancel buttons are still visible (and scrolling is minimized.) As a result, I did things like the Track Load Options where two of them can fit on the same line instead of adding another row. You're right, it's a heading, but the space saving I think is worth it (since not seeing the Apply/Cancel buttons is a far worse problem.) We could change that heading to "On track load:" leading the user to continue to read to complete the sentence, but I'm not sure if that will translate as well as "Track load options." What do you think? With the Speed/Pitch options, I also thought about doing a horizontal line like under the skin options, but that again takes up an extra row. In this case, the options are all related to speed and pitch, so a groupbox makes sense and allows the heading and separator to share a row. I struggled with the Slider Range combo box size, because the parenthetical notes would look better if it was wider, but the important parts are only three characters wide. So I made the choice to reduce it, again to save a row, being able to fit the slider direction next to it instead of under it. I added a tooltip on that as well to explain that it's the turntable default. Bend Behavior only has two options. (I could even reduce it to a single checkbox, bend or not, but two radio buttons is clearer.) |
The Apply/Cancel buttons are always visible even on my eeePC since they are on the non scrolling footer. Of cause scrolling should be avoided. For me the whole preference window is ugly and not of high usability, because it misses a general pattern. Every single pane has a different idea how to arrange the options. I think Android has such patterns and so the preference windows are very similar. IMHO this is more important than saving horizontal space. With my comments I have tried to condense your ideas to some patterns.
This would be OK for me once we have such patterns and verified they will finally fit to all panes. What are your ideas for the whole picture? |
|
My plans are to look at each pane independently and use whatever GUI elements best fit the config options on each and take up the least vertical space. |
This doesn't work because Mixxx (master and 2.0) currently appears to auto-apply settings changes. (We should remove the Cancel button if this is intended behavior.) |
|
I think an incremental improvement in the preference pane is acceptable. I am ok with this PR in general and leaving a revamp of the whole pref system for later. Also it is not the job of this PR to fix the problem we have with not-always-instant-apply, as long as the change doesn't introduce new issues. I'll try this out when I get a chance! |
Conflicts: src/mixxx.cpp
| m_pConfig->set(ConfigKey("[Controls]", "PositionDisplay"),ConfigValue(0)); | ||
| // If not present in the config, set the default value | ||
| if (!m_pConfig->exists(ConfigKey("[Controls]","PositionDisplay"))) | ||
| m_pConfig->set(ConfigKey("[Controls]","PositionDisplay"),ConfigValue(1)); |
There was a problem hiding this comment.
Did you intend to change the default here?
There was a problem hiding this comment.
Ah, no. Did it change in master at some point in the last couple years?
| // Speed auto reset combobox | ||
| m_speedAutoReset = BaseTrackPlayer::RESET_PITCH; | ||
| ComboBoxResetSpeedAndPitch->setCurrentIndex(BaseTrackPlayer::RESET_PITCH); | ||
| // Don't automatically reset the pitch & speed on track load |
|
off; library only; Skin and library |
|
@ywwg Longer translations (see Sebastien's example above) are a good reason NOT to make prefs panes two groupboxes wide. (So there's room for them to expand horizontally.) |
I had a feeling "peau" was not the right translation :). |
|
"peau" is skin like in body skin. In French we use the word "thème" for software interface skin. |
| ConfigKey("[Controls]", "ShowDurationRemaining")); | ||
| connect(m_pControlPositionDisplay, SIGNAL(valueChanged(double)), | ||
| connect(m_pControlTrackTimeDisplay, SIGNAL(valueChanged(double)), | ||
| this, SLOT(slotSetPositionDisplay(double))); |
There was a problem hiding this comment.
This slot no longer exists since hte function is renamed
|
@Pegasus-RPG
|
|
@daschuer The columns in the group box are independent from the ones above, so they will almost never align. The solution is to move those options to their own pane, as mentioned previously. Regarding the group box heading, it sounds like you dislike the way Cinnamon chooses to render those. (You can see on my KDE screen shots that the text is clearly a group box heading.) We can't worry about how different desktops choose to render standard GUI elements or we'll forever be hacking things to look good in one case and messing up others. |
Will we have this before 2.1? If not, I think it is worth to polish it for all common target systems. The library pane looks good on all targets. It is because it has group boxes for all sections. Any thoughts? |
|
I think Daniel's screenshot looks fine, I see no reason to make any changes. |
|
For my eyes, this looks better and is consistent with the other panes.
|
|
An other issue is, that I did not noticed that the waveform preferences are now hidden behind a [+] |
|
is that a mockup or is it a code change? It just looks like a different desktop theme |
|
Both screen shots are taken form QT Designer. One with GTK style, the other with a style rendering the group box lines. |
|
style choice is up to the user, it's not up to us to decide that we prefer one or the other. |
|
sure ... It should only look reasonable nice on each default style our target OSs. |
|
GTK does not use boxes for groups by default, therefore it would look inconsistent to add them. PR is fine as is. |
|
I did not propose to use boxes = "lines around groups" in GTK. I am just trying to find a solution that looks nice with these lines and without, depending on the chosen render style. Please compare the GTK screen-shots of today with the GTK screen shot from yesterday. Both new ones are nicer for my eyes and are matching with the concepts used in the majority of the other panes like the library pane. |
|
We already had this discussion, why are you bringing it up again? "It feels like we're forcing things into 3 groupboxes when they don't fit. Given the expense of vertical height for this decoration I would suggest to wait on groupboxes until we split the panes." |
|
I have played around with it for a while. An I did not find a good solution with a single group box. Than I realised, that we had the "Unrelated topics" issue in the other panes as well. There we have solved it by a miscellaneous group. And what works there might work here as well. So I have decided to show it here as a draft. As said any other low impact solution is welcome too. |
…j/mixxx#527 Reverts the last revert, now how it should have been from the beginning, with correct markdown syntax for crosslinking on Github
Optimized the Interface preferences pane:
No ConfigKey names were changed (other than an incorrect one, fixing a bug) so this is compatible with existing configurations, and all changed items were manually tested against existing config file values to ensure consistency. I also manually ensured the changed items indeed have the desired effect in Mixxx and that they get correctly saved to the config file, as well as display appropriately when loaded from it on startup.
Don't sweat the size of the diff for the .ui file. This happens mostly because Qt Designer rearranges the order of the XML elements on every save, you know, like Mixxx does with its controller mapping files. ;)
Before:
After:
Feel free to suggest rearrangements.