-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Color preferences improvements #2589
Merged
Merged
Changes from 5 commits
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
a653006
Improve strings in the color preferences pane
daschuer 29158a5
Allow to translate "Name" in the ColorPaletteEditor
daschuer 7e1be9e
Move button row below the palette and optimize column widths
daschuer 30aaea8
Fill table with assigned cue numbers if no cue numbers are set
daschuer 3b66047
Introduce Compbobox for selecting the hotcue default color
daschuer d990c5e
Respect new hotcue default settings
daschuer c53f7c1
Added a preview stripe for the selected palette.
daschuer b4d3fa5
Fix default default hotcue selection
daschuer 04ed468
Revert "Fill table with assigned cue numbers if no cue numbers are set"
daschuer a171d13
Improve the labels of the colors in the combobox
daschuer 68742d9
introduce getHotcueColorPalette(const QString&) and make use of it.
daschuer f9714bb
Added Icons to the palette select boxes
daschuer 539d4da
Limit auto color asignment to 8
daschuer 7b15170
DlgPrefColors: use full palette preview for icons in comboboxes
Be-ing 5d4321f
limit default hotcue palette to 8 colors
Be-ing 75b3af5
Merge remote-tracking branch 'upstream/master' into color_preferences
daschuer edc4af6
Rename Intro Palette to Track Metadata und remove it from the user av…
daschuer 3ab4f63
Inital hide the palette editor
daschuer 5cb7e2f
Split Save as and Template Combobox
daschuer 326008d
Merge remote-tracking branch 'upstream/pr/2589' into color_preferences
Be-ing b267a44
DlgPrefColors: remove unused slotTrackPaletteChanged
Be-ing 4176013
DlgPrefColors: fix black edge of palette preview pixmaps
Be-ing 9c5cd43
Merge pull request #50 from Be-ing/color_preferences
daschuer fa3cac9
Use small edit button with elipsis
daschuer 089fc08
Remove superfluid parenthes
daschuer 61ecb24
Added message box when closing Palette Editor wit unsaved changes
daschuer 37a57b7
Keep temoraray selections when updateding custom paletts
daschuer 3d68315
Keep temorary selected default color after chanigng palette
daschuer 73ee264
Improve gray out state of save and reset button
daschuer 8e95102
Improve naming and use int as index like Qt
daschuer 5f601ce
Made the Palette editor a context depending pop up box for the palett…
daschuer e9c2d34
Seti inital color for QColorDialog
daschuer 74b47c3
Don't translate parentheses
daschuer 441b069
Adjust default index when it is out set the new palette selected
daschuer f1a7c73
fix typo
daschuer 989d335
ColorPaletteEditor: move Add/Remove Color to buttons
Be-ing 2a8f635
ColorPaletteEditor: hide hotcue number column for track palette
Be-ing d5e217a
Merge pull request #51 from Be-ing/color_preferences
daschuer 61f5a71
Resort the palette editor buttons. Replace "Save" with "OK"
daschuer f189fee
Shrink add and remove button to + and - and move it below the table.
daschuer 4030280
Improve selection behaviour during palette edit
daschuer 4b3f9b4
Added a warning dialog when removing the palette.
daschuer a82bf80
Remove commented code
daschuer cfce951
Swap "+" and "-" button
daschuer f29e3b8
Added some comments
daschuer 3126268
Merge pull request #52 from Be-ing/color_preferences_hide_hotcue_number
daschuer 9db6ad2
Use the real hotcue colors when coloring the icon
daschuer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,19 @@ | ||
#include "preferences/dialog/dlgprefcolors.h" | ||
|
||
#include <QColorDialog> | ||
#include <QPainter> | ||
#include <QStandardItemModel> | ||
#include <QtDebug> | ||
|
||
#include "control/controlobject.h" | ||
#include "util/color/predefinedcolorpalettes.h" | ||
|
||
namespace { | ||
|
||
constexpr int kHotcueDefaultColorIndex = -1; | ||
|
||
} // anonymous namespace | ||
|
||
DlgPrefColors::DlgPrefColors( | ||
QWidget* parent, UserSettingsPointer pConfig) | ||
: DlgPreferencePage(parent), | ||
|
@@ -47,12 +54,44 @@ void DlgPrefColors::loadSettings() { | |
comboBoxTrackColors->addItem(paletteName); | ||
} | ||
|
||
const ColorPalette hotcuePalette = m_colorPaletteSettings.getHotcueColorPalette(); | ||
|
||
comboBoxHotcueColors->setCurrentText( | ||
m_colorPaletteSettings.getHotcueColorPalette().getName()); | ||
hotcuePalette.getName()); | ||
comboBoxTrackColors->setCurrentText( | ||
m_colorPaletteSettings.getTrackColorPalette().getName()); | ||
|
||
slotApply(); | ||
QPixmap pixmap(80, 80); | ||
QPainter painter(&pixmap); | ||
pixmap.fill(Qt::black); | ||
|
||
comboBoxHotcueDefaultColor->addItem(tr("By hotcue number"), -1); | ||
|
||
for (int i = 0; i < hotcuePalette.size() && i < 4; ++i) { | ||
painter.setBrush(mixxx::RgbColor::toQColor(hotcuePalette.at(i))); | ||
painter.drawRect(0, i * 20, 80, 20); | ||
} | ||
comboBoxHotcueDefaultColor->setItemIcon(0, QIcon(pixmap)); | ||
|
||
for (int i = 0; i < hotcuePalette.size(); ++i) { | ||
comboBoxHotcueDefaultColor->addItem(tr("Palette") + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Palette N" is not the right term, why no use something like "Color N: #FF8000"? |
||
QStringLiteral(" ") + QString::number(i + 1), | ||
i); | ||
pixmap.fill(mixxx::RgbColor::toQColor(hotcuePalette.at(i))); | ||
comboBoxHotcueDefaultColor->setItemIcon(i + 1, QIcon(pixmap)); | ||
} | ||
|
||
bool autoHotcueColors = | ||
m_pConfig->getValue(ConfigKey("[Controls]", "auto_hotcue_colors"), false); | ||
if (autoHotcueColors) { | ||
comboBoxHotcueDefaultColor->setCurrentIndex(0); | ||
} else { | ||
int hotcueDefaultColorIndex = m_pConfig->getValue(ConfigKey("[Controls]", "HotcueDefaultColorIndex"), kHotcueDefaultColorIndex); | ||
if (hotcueDefaultColorIndex < 0 || hotcueDefaultColorIndex >= hotcuePalette.size()) { | ||
hotcueDefaultColorIndex = hotcuePalette.size() - 1; // default to last color (orange) | ||
} | ||
comboBoxHotcueDefaultColor->setCurrentIndex(hotcueDefaultColorIndex + 1); | ||
} | ||
} | ||
|
||
// Set the default values for all the widgets | ||
|
@@ -61,6 +100,7 @@ void DlgPrefColors::slotResetToDefaults() { | |
mixxx::PredefinedColorPalettes::kDefaultHotcueColorPalette.getName()); | ||
comboBoxTrackColors->setCurrentText( | ||
mixxx::PredefinedColorPalettes::kDefaultTrackColorPalette.getName()); | ||
comboBoxHotcueDefaultColor->setCurrentIndex(1); | ||
slotApply(); | ||
} | ||
|
||
|
@@ -94,7 +134,13 @@ void DlgPrefColors::slotApply() { | |
m_colorPaletteSettings.getTrackColorPalette())); | ||
} | ||
|
||
m_pConfig->setValue( | ||
ConfigKey("[Controls]", "auto_hotcue_colors"), | ||
checkBoxAssignHotcueColors->isChecked()); | ||
int index = comboBoxHotcueDefaultColor->currentIndex(); | ||
|
||
if (index > 0) { | ||
m_pConfig->setValue(ConfigKey("[Controls]", "auto_hotcue_colors"), false); | ||
m_pConfig->setValue(ConfigKey("[Controls]", "HotcueDefaultColorIndex"), index - 1); | ||
} else { | ||
m_pConfig->setValue(ConfigKey("[Controls]", "auto_hotcue_colors"), true); | ||
m_pConfig->setValue(ConfigKey("[Controls]", "HotcueDefaultColorIndex"), -1); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it's a bit cumbersome so set a single default color for a large palette because you'd have to remove all the other numbers in the index column by hand. It's also a bit confusing because we now have numbers for the built-in track color palettes, too. The previous situation was probably also confusing, so I'm not sure which one is better. If we keep this change I think we need a context menu item to unset all indices in the palette editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to have always the coloumn populated with the cue numbers and enable the use via the box.
So you only need to edit the column if you are not fine with this. This should effect virtually no one, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say I have a palette with 10 colors. One of the colors is red. If I want all my new cues red, I have to set "assign to hotcue number" to 1 for red and an empty value for all others. Before, I could just double click next to the red color and type 1. Done.
Now I have to the the same and then delete the numbers from all other rows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be move into the paletteditor(model) classes. If there are no assigned hotcue indices, we could display grey placeholder values in the "assign to hot cue numbers" table (the placeholder values are not saved) . As soon as you set one value eyplicitly, the placeholder values should disappear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. That was not self explaining to me. I have actually tried to add all cue numbers to red like 1, 2, 3 ... but it fails.
A combobox with all palette colors + color by number for default cue color will be IMHO quite easy to understand compared to that.
I like to avoid to bothering the user with the palette editor if he just want to select the default color.
Does this work for you as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that setting the default color via the palette editor causes too much friction, however let's keep this out of this PR as I have an idea for that that I'd like to try in a separate PR after this one has been merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you implement this? The current change also prevent reordering the palette with no indices set. Also, palette could be big so you should check if
i
is less than the number of hotcues.