Skip to content
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.

Sign up for GitHub

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 47 commits into from
Apr 5, 2020
Merged

Color preferences improvements #2589

Show file tree
Hide file tree
Changes from 4 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 Mar 22, 2020
29158a5
Allow to translate "Name" in the ColorPaletteEditor
daschuer Mar 22, 2020
7e1be9e
Move button row below the palette and optimize column widths
daschuer Mar 22, 2020
30aaea8
Fill table with assigned cue numbers if no cue numbers are set
daschuer Mar 22, 2020
3b66047
Introduce Compbobox for selecting the hotcue default color
daschuer Mar 24, 2020
d990c5e
Respect new hotcue default settings
daschuer Mar 24, 2020
c53f7c1
Added a preview stripe for the selected palette.
daschuer Mar 24, 2020
b4d3fa5
Fix default default hotcue selection
daschuer Mar 24, 2020
04ed468
Revert "Fill table with assigned cue numbers if no cue numbers are set"
daschuer Mar 24, 2020
a171d13
Improve the labels of the colors in the combobox
daschuer Mar 24, 2020
68742d9
introduce getHotcueColorPalette(const QString&) and make use of it.
daschuer Mar 24, 2020
f9714bb
Added Icons to the palette select boxes
daschuer Mar 24, 2020
539d4da
Limit auto color asignment to 8
daschuer Mar 24, 2020
7b15170
DlgPrefColors: use full palette preview for icons in comboboxes
Be-ing Mar 25, 2020
5d4321f
limit default hotcue palette to 8 colors
Be-ing Mar 25, 2020
75b3af5
Merge remote-tracking branch 'upstream/master' into color_preferences
daschuer Mar 26, 2020
edc4af6
Rename Intro Palette to Track Metadata und remove it from the user av…
daschuer Mar 26, 2020
3ab4f63
Inital hide the palette editor
daschuer Mar 26, 2020
5cb7e2f
Split Save as and Template Combobox
daschuer Mar 26, 2020
326008d
Merge remote-tracking branch 'upstream/pr/2589' into color_preferences
Be-ing Mar 27, 2020
b267a44
DlgPrefColors: remove unused slotTrackPaletteChanged
Be-ing Mar 27, 2020
4176013
DlgPrefColors: fix black edge of palette preview pixmaps
Be-ing Mar 27, 2020
9c5cd43
Merge pull request #50 from Be-ing/color_preferences
daschuer Mar 27, 2020
fa3cac9
Use small edit button with elipsis
daschuer Mar 27, 2020
089fc08
Remove superfluid parenthes
daschuer Mar 27, 2020
61ecb24
Added message box when closing Palette Editor wit unsaved changes
daschuer Mar 27, 2020
37a57b7
Keep temoraray selections when updateding custom paletts
daschuer Mar 27, 2020
3d68315
Keep temorary selected default color after chanigng palette
daschuer Mar 27, 2020
73ee264
Improve gray out state of save and reset button
daschuer Mar 27, 2020
8e95102
Improve naming and use int as index like Qt
daschuer Mar 28, 2020
5f601ce
Made the Palette editor a context depending pop up box for the palett…
daschuer Mar 29, 2020
e9c2d34
Seti inital color for QColorDialog
daschuer Mar 29, 2020
74b47c3
Don't translate parentheses
daschuer Mar 29, 2020
441b069
Adjust default index when it is out set the new palette selected
daschuer Mar 29, 2020
f1a7c73
fix typo
daschuer Mar 29, 2020
989d335
ColorPaletteEditor: move Add/Remove Color to buttons
Be-ing Mar 30, 2020
2a8f635
ColorPaletteEditor: hide hotcue number column for track palette
Be-ing Mar 30, 2020
d5e217a
Merge pull request #51 from Be-ing/color_preferences
daschuer Mar 30, 2020
61f5a71
Resort the palette editor buttons. Replace "Save" with "OK"
daschuer Mar 30, 2020
f189fee
Shrink add and remove button to + and - and move it below the table.
daschuer Mar 30, 2020
4030280
Improve selection behaviour during palette edit
daschuer Mar 30, 2020
4b3f9b4
Added a warning dialog when removing the palette.
daschuer Mar 30, 2020
a82bf80
Remove commented code
daschuer Mar 31, 2020
cfce951
Swap "+" and "-" button
daschuer Mar 31, 2020
f29e3b8
Added some comments
daschuer Mar 31, 2020
3126268
Merge pull request #52 from Be-ing/color_preferences_hide_hotcue_number
daschuer Apr 2, 2020
9db6ad2
Use the real hotcue colors when coloring the icon
daschuer Apr 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/preferences/colorpaletteeditor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,20 +31,21 @@ ColorPaletteEditor::ColorPaletteEditor(QWidget* parent)
m_pDiscardButton = pButtonBox->addButton(QDialogButtonBox::Discard);

QHBoxLayout* pTopLayout = new QHBoxLayout();
pTopLayout->addWidget(new QLabel("Name:"));
pTopLayout->addWidget(new QLabel(tr("Name")));
pTopLayout->addWidget(m_pPaletteNameComboBox, 1);
pTopLayout->addWidget(pButtonBox);

QVBoxLayout* pLayout = new QVBoxLayout();
pLayout->addLayout(pTopLayout);
pLayout->addWidget(m_pTableView, 1);
pLayout->addWidget(pButtonBox);
setLayout(pLayout);
setContentsMargins(0, 0, 0, 0);

// Set up model
m_pModel->setColumnCount(2);
m_pModel->setColumnCount(3);
m_pModel->setHeaderData(0, Qt::Horizontal, tr("Color"), Qt::DisplayRole);
m_pModel->setHeaderData(1, Qt::Horizontal, tr("Assign to Hotcue"), Qt::DisplayRole);
m_pModel->setHeaderData(1, Qt::Horizontal, tr("Assign to Hotcue Number"), Qt::DisplayRole);
m_pModel->setHeaderData(2, Qt::Horizontal, QString(), Qt::DisplayRole);
connect(m_pModel,
&ColorPaletteEditorModel::dirtyChanged,
this,
Expand All @@ -63,6 +64,10 @@ ColorPaletteEditor::ColorPaletteEditor(QWidget* parent)
m_pTableView->setContextMenuPolicy(Qt::CustomContextMenu);
m_pTableView->setModel(m_pModel);

m_pTableView->horizontalHeader()->setSectionResizeMode(0, QHeaderView::ResizeToContents);
m_pTableView->horizontalHeader()->setSectionResizeMode(1, QHeaderView::ResizeToContents);
m_pTableView->horizontalHeader()->setSectionResizeMode(2, QHeaderView::Stretch);

connect(m_pTableView,
&QTableView::doubleClicked,
this,
Expand Down
12 changes: 9 additions & 3 deletions src/preferences/colorpaletteeditormodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,15 @@ void ColorPaletteEditorModel::setColorPalette(const ColorPalette& palette) {
// Make a map of hotcue indices
QMap<int, int> hotcueColorIndicesMap;
QList<unsigned int> hotcueColorIndices = palette.getHotcueIndices();
for (int i = 0; i < hotcueColorIndices.size(); i++) {
int colorIndex = hotcueColorIndices.at(i);
hotcueColorIndicesMap.insert(colorIndex, i);
if (hotcueColorIndices.size()) {
for (int i = 0; i < hotcueColorIndices.size(); i++) {
int colorIndex = hotcueColorIndices.at(i);
hotcueColorIndicesMap.insert(colorIndex, i);
}
} else {
for (int i = 0; i < palette.size(); i++) {
hotcueColorIndicesMap.insert(i, i);
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before, I could just double click next to the red color and type 1. Done.

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?

Copy link
Member

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.

Copy link
Member

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 explicitly, the placeholder values should disappear.

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.

}
}

Be-ing marked this conversation as resolved.
Show resolved Hide resolved
for (int i = 0; i < palette.size(); i++) {
Expand Down
28 changes: 21 additions & 7 deletions src/preferences/dialog/dlgprefcolorsdlg.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<rect>
<x>0</x>
<y>0</y>
<width>524</width>
<width>656</width>
<height>333</height>
</rect>
</property>
Expand All @@ -29,27 +29,41 @@
<item row="0" column="0">
<widget class="QLabel" name="labelHotcueColors">
<property name="text">
<string>Hotcues:</string>
<string>Hotcue Palette</string>
</property>
</widget>
</item>
<item row="0" column="1">
<widget class="QComboBox" name="comboBoxHotcueColors"/>
<widget class="QComboBox" name="comboBoxHotcueColors">
<property name="sizePolicy">
<sizepolicy hsizetype="Expanding" vsizetype="Fixed">
<horstretch>0</horstretch>
<verstretch>0</verstretch>
</sizepolicy>
</property>
</widget>
</item>
<item row="1" column="0">
<widget class="QLabel" name="labelTrackColors">
<property name="text">
<string>Track:</string>
<string>Track Palette</string>
</property>
</widget>
</item>
<item row="1" column="1">
<widget class="QComboBox" name="comboBoxTrackColors"/>
<widget class="QComboBox" name="comboBoxTrackColors">
<property name="sizePolicy">
<sizepolicy hsizetype="Expanding" vsizetype="Fixed">
<horstretch>0</horstretch>
<verstretch>0</verstretch>
</sizepolicy>
</property>
</widget>
</item>
<item row="2" column="0">
<widget class="QLabel" name="labelAutoHotcueColors">
<property name="text">
<string>Auto hotcue colors</string>
<string>Cycle hotcue colors</string>
</property>
<property name="buddy">
<cstring>checkBoxAssignHotcueColors</cstring>
Expand All @@ -62,7 +76,7 @@
<string>Automatically assigns a predefined color to a newly created hotcue point, based on its index.</string>
</property>
<property name="text">
<string>Assign predefined colors to newly created hotcue points</string>
<string>Select consecutive palette colors for new hotcues</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The palette colors aren't necessarily consecutive as they can be in any order depending on the "Assign to Hotcue Index" column. However, I'd suggest we remove this option completely - see PR #2578,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is very usable to have this option separate from the palette editor.
Many users probably fine with the Mixxx colors and only like to select the default color or the color by number option.
So I actually want the combobox back where we can select one or all colors from the current palette.
I think you are used to the color by number feature, right?
I like to have one default color. Currently the later is hidden and therefore unpredictable.
We need to fix that as well.
Would it OK for you to bring that box back?

</property>
</widget>
</item>
Expand Down