BPM Lock Icon

Bug #992811 reported by RJ Skerry-Ryan on 2012-05-01
14
This bug affects 3 people
Affects Status Importance Assigned to Milestone
Mixxx
Low
Max Linke

Bug Description

The new BPM lock feature in 1.11 adds a checkbox next to each BPM in the library table. Currently it's the same checkbox as the played one. It would be better if we had a nice lock icon.

Related branches

RJ Skerry-Ryan (rryan) on 2012-05-01
Changed in mixxx:
milestone: none → 1.11.0
assignee: nobody → jus (jus)
importance: Undecided → Low
status: New → Confirmed
jus (jus) wrote :

I need a little help with this one please.
With the current code (e.g. around line 2138 in Deere1280x800-WXGA/skin.xml ) all checkbox are the same, but we want to have separate icons for played/bpm lock/whatever column.
QTableView::indicator { width: 12px; height: 12px;}
QTableView::indicator:checked { background: url(skin:/foo.png);}
QTableView::indicator:unchecked { background: url(skin:/foo.png);}

I tried to specify an ID Selector with no luck. It should match all QTableView instances whose object name is LIBRARYTABLE_BPM_LOCK. I can work with LIBRARYTABLE_BPM_LOCK, can i?
QTableView#LIBRARYTABLE_BPM_LOCK::indicator { width: 12px; height: 12px;}
QTableView#LIBRARYTABLE_BPM_LOCK::indicator:checked { background: url(skin:/foo.png);}
QTableView#LIBRARYTABLE_BPM_LOCK::indicator:unchecked { background: url(skin:/foo.png);}

Hm -- sorry for asking without making sure it was possible :). I'll take a
look tonight.

On Wed, May 2, 2012 at 4:01 AM, jus <email address hidden> wrote:

> I need a little help with this one please.
> With the current code (e.g. around line 2138 in
> Deere1280x800-WXGA/skin.xml ) all checkbox are the same, but we want to
> have separate icons for played/bpm lock/whatever column.
> QTableView::indicator { width: 12px; height: 12px;}
> QTableView::indicator:checked { background: url(skin:/foo.png);}
> QTableView::indicator:unchecked { background: url(skin:/foo.png);}
>
> I tried to specify an ID Selector with no luck. It should match all
> QTableView instances whose object name is LIBRARYTABLE_BPM_LOCK. I can work
> with LIBRARYTABLE_BPM_LOCK, can i?
> QTableView#LIBRARYTABLE_BPM_LOCK::indicator { width: 12px; height: 12px;}
> QTableView#LIBRARYTABLE_BPM_LOCK::indicator:checked { background:
> url(skin:/foo.png);}
> QTableView#LIBRARYTABLE_BPM_LOCK::indicator:unchecked { background:
> url(skin:/foo.png);}
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/992811
>
> Title:
> BPM Lock Icon
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/992811/+subscriptions
>

Changed in mixxx:
assignee: jus (jus) → RJ Ryan (rryan)
RJ Skerry-Ryan (rryan) wrote :

I don't think we can change this without a lot of work :(.

Changed in mixxx:
status: Confirmed → Triaged
Max Linke (max-linke) wrote :

This should be fairly easy to do. We just need to write a new custom delegate (afaik that's the prefered way in Qt to do stuff like this). You could take the previewbutton class in my previewdeck-branch as an example. The Button should then just show the desired lock icon instead of the playbutton. The only downside so far is that I haven't figured out how to make this icon skinable to it would be hardcoded.

RJ Skerry-Ryan (rryan) on 2012-11-21
Changed in mixxx:
milestone: 1.11.0 → 1.11.1
Max Linke (max-linke) wrote :

Are your actively working on this Ryan?

I started a couple of days ago to write a custom delegate for the BPM column to also another bug.

RJ Skerry-Ryan (rryan) wrote :

Oh great. I am not workign on it. If you think it might be done soon then
we can retarget for 1.11.0. I just assumed the progress was slow going and
was trying to clear up the 1.11.0 milestone to only have bugs we plan to
finish for 1.11.0 in it.

On Wed, Nov 21, 2012 at 4:22 AM, Max Linke <email address hidden>wrote:

> Are your actively working on this Ryan?
>
> I started a couple of days ago to write a custom delegate for the BPM
> column to also another bug.
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/992811
>
> Title:
> BPM Lock Icon
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/992811/+subscriptions
>

Max Linke (max-linke) wrote :

Ok

Leave the 1.11.1 Target. I've started a couple of days ago and I will need some time to figure out SizeHints in Qt so that everything is scaled right and fits.

Changed in mixxx:
assignee: RJ Ryan (rryan) → Max Linke (max-linke)
Max Linke (max-linke) wrote :

I've created a new branch for this '~max-linke/mixxx/bpmdelegate'

I've subclassed QAbstractButton to implement my own Checkbox that can be filled with whatever picture is desired. But I have problems to correctly reimplement the paintEvent. Right now nothing is. Some hints on how to get this working would be nice

Max Linke (max-linke) wrote :

I think my branch is almost done. Can anyone test if it works as expected.

Changed in mixxx:
status: Triaged → In Progress
jus (jus) wrote :

Works well here, thanks for implementing the enhancement.

From a user perspective, i would expect the "Lock BPM" option to grey out if the BPM is locked. And vice versa if a the BPM is unlocked, "Unlock BPM" to grey out in the context menu.

And something that bugged me already with the Preview button - Depending on the skin/color scheme, hardcoded icons are sometimes hard to see. Can we style the BPM icon/Preview button using stylesheets in the skin, like we do for the "Played" checkmark?

RJ Skerry-Ryan (rryan) wrote :

Currently we look for the play icon in res/images/library/ic_library_preview_play.png.

What if we first look for the icon in <skinpath>/library/ic_library_preview_play.png ?
That would let skins pick their own icon that suits the skin coloring.

Styling the widget is also probably possible. If we can get the QPushButton to be parented to the Library widget (even indirectly) then it should inherit the style.

RJ Skerry-Ryan (rryan) wrote :

It looks like the QPushButton is already parented to the table view. Jus -- have you tried putting QPushButton styles in the Library <Style> tag?

Yesterday i followed http://doc.qt.digia.com/main-snapshot/stylesheet-examples.html#complex-selector-example and managed that all buttons in the Tableview used the same style. But i could not find the correct selector/object name to only style the Preview button. Maybe just a syntax ting.

On Nov 30, 2012, at 5:08 PM, RJ Ryan <email address hidden> wrote:

> It looks like the QPushButton is already parented to the table view. Jus
> -- have you tried putting QPushButton styles in the Library <Style> tag?
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/992811
>
> Title:
> BPM Lock Icon
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/992811/+subscriptions

RJ Skerry-Ryan (rryan) wrote :

Just tested and this works to style the button:

<Library>
<Style>
QPushButton {
background-color: red;
}
QPushButton:hover {
background-color: green;
}
</Style>
</Library>

I'm also adding object names to a bunch of library widgets so you can style it like:

#LibraryPreviewButton {

}

I'll document all the object-names in the skin wiki page. This way if we have multiple push button delegates you aren't addressing them all by using QPushButton.

BTW it looks like QPushButton supports setting its image via CSS:

QPushButton:pressed { image: url(':images/settings_press.png') }

http://stackoverflow.com/questions/12391125/qpushbutton-css-pressed

The problem is we can't change the image based on the play or pause state of the button via CSS. Maybe we could define a cssClass property for widgets like this so that it could dynamically change its class. That way skins could say

#LibraryPushButton.pause {
  image: url(...);
}

#LibraryPushButton.play {
  image: url(...);
}

Seems possible according to:
http://stackoverflow.com/questions/4596903/css-selector-for-custom-qt-class

RJ Skerry-Ryan (rryan) wrote :

Nope, you can't style just the preview button I think without the object
name change I just made in r3533.

You should be able to do that now with #LibraryPreviewButton {
background-color: red; }

On Fri, Nov 30, 2012 at 11:32 AM, jus <email address hidden> wrote:

> Yesterday i followed http://doc.qt.digia.com/main-snapshot/stylesheet-
> examples.html#complex-selector-example and managed that all buttons in
> the Tableview used the same style. But i could not find the correct
> selector/object name to only style the Preview button. Maybe just a
> syntax ting.
>
> On Nov 30, 2012, at 5:08 PM, RJ Ryan <email address hidden> wrote:
>
> > It looks like the QPushButton is already parented to the table view. Jus
> > -- have you tried putting QPushButton styles in the Library <Style> tag?
> >
> > --
> > You received this bug notification because you are a member of Mixxx
> > Development Team, which is subscribed to Mixxx.
> > https://bugs.launchpad.net/bugs/992811
> >
> > Title:
> > BPM Lock Icon
> >
> > To manage notifications about this bug go to:
> > https://bugs.launchpad.net/mixxx/+bug/992811/+subscriptions
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/992811
>
> Title:
> BPM Lock Icon
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/992811/+subscriptions
>

Max Linke (max-linke) wrote :

About the look icons. I was hoping jus could give me some I can use. To grey out the icon it should be possible to play with the rendering in the paintEvent.

Setting the image with the CSS style for the BPM Icon could be a bit mor difficult, since I subclassed QAbstractButton to write my own paintEvent. And I would also like to change the previewbutton into a subclass from QAbstractButton.

I wonder if you could replicate the QPushButton paintEvent to get styling:
http://qt.gitorious.org/qt/qtbase/blobs/1dc517abc688f6cf20bdc75e8a4ff2dda86b5d70/src/widgets/widgets/qpushbutton.cpp#line452

Another similar solution:
http://stackoverflow.com/questions/7276330/qt-stylesheet-for-custom-widget

On Fri, Nov 30, 2012 at 11:58 AM, Max Linke <email address hidden>wrote:

> About the look icons. I was hoping jus could give me some I can use. To
> grey out the icon it should be possible to play with the rendering in
> the paintEvent.
>
> Setting the image with the CSS style for the BPM Icon could be a bit mor
> difficult, since I subclassed QAbstractButton to write my own
> paintEvent. And I would also like to change the previewbutton into a
> subclass from QAbstractButton.
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/992811
>
> Title:
> BPM Lock Icon
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/992811/+subscriptions
>

jus (jus) wrote :

On Nov 30, 2012, at 5:58 PM, Max Linke <email address hidden> wrote:

> About the look icons. I was hoping jus could give me some I can use. To
> grey out the icon it should be possible to play with the rendering in
> the paintEvent.

@Max
Before there is a misunderstanding here, in my post #10 i referred to the right-click mouse context menu, not the icon.
So that for example the words "Unlock BPM" are greyed out, just like we did for "Clear BPM and Beatgrid".

RJ Skerry-Ryan (rryan) wrote :

I've been thinking a little bit about this lately and I think that subclassing QAbstractButton is the wrong way to go since it's unlikely we will be able to get the style sheet support right. In general we should move away from the old way of doing widgets in Mixxx where we override paint events and do anything related to palettes since this leads to inconsistencies in the UI.

Everything should be styleable with style sheets. I think the icons for all our delegates in the library shouldn't even be hardcoded.

For example, this works if you uncomment the hardcoding of the preview button's pause image and make the check-state of the preview button reflect whether the track is the current previewed track.

#LibraryPreviewButton {
  background-color: red;
  border: 0;
}

#LibraryPreviewButton:checked {
  image: url(skin:/style/checked.png);
}

#LibraryPreviewButton:!checked {
  image: url(skin:/style/unchecked.png);
}

#LibraryPreviewButton:checked:hover {
  image: url(skin:/style/unchecked.png);
}

#LibraryPreviewButton:!checked:hover {
  image: url(skin:/style/checked.png);
}

This is much more configurable since the skin designer can set the graphics and even do things like set a custom border on hover so that you know it's a button. (since border:0 makes there be no outline of a button around it).

I think that for 2-state buttons like the BPM lock and the preview button a QPushButton that is checkable is good enough.

RJ Skerry-Ryan (rryan) wrote :

Now lp:mixxx/1.11 has the check behavior so the above CSS works except for the hardcoded play button. We can remove it but the downside is using a skin from 1.10.x (including all community made skins) will not show any icon at all for the preview button and I don't have a way to tell if the CSS would style it to have a button.

RJ Skerry-Ryan (rryan) wrote :

I've removed the default Icon and now insert a default CSS style in LegacySkinParser for TableView. This selects for legacy skins since only legacy skins will use <TableView>. In either case, the skin writer can over-ride the default CSS using <Style>. I added a placeholder pause image because I assume jus will swap out the ic_library_preview_play.png and ic_library_preview_pause.png with something else.

Max Linke (max-linke) wrote :

Oh cool. I tried to find a way to do this with the previewButton. Through lack of knowledge I was drawn to rewrite the paintEvent. I'll try to do the same with the BPM lock button.

@jus
How would you like it to handle the case where several tracks that have different lock settings are selected ?

Max Linke (max-linke) wrote :

I've changed the BPM delegate that is also uses style sheets to set the images.

For LateNight this works perfect. In Outline not so much. All works great in the previewButton though. I guess I need to use the style sheets also for the QLabel and QDoubleSpinBox.

Max Linke (max-linke) wrote :
Max Linke (max-linke) wrote :

After playing around a bit I think the problem with the background of the row is because the style sheets have huge differences in some parts about the colors. Why are there colorschemes definded in the outline xml?

I've also tried to style the QDoubleSpinBox with the same code as for the QSpinBox, does not work -.-

RJ Skerry-Ryan (rryan) wrote :

I think we are going to get rid of color schemes at some point. Outline is
the only skin left that uses them.

For the preview button I've found background:transparent on the QPushButton
was the only way to get the alternating colors from the library rows to
show up.

On Tue, Dec 4, 2012 at 11:26 AM, Max Linke <email address hidden>wrote:

> After playing around a bit I think the problem with the background of
> the row is because the style sheets have huge differences in some parts
> about the colors. Why are there colorschemes definded in the outline
> xml?
>
> I've also tried to style the QDoubleSpinBox with the same code as for
> the QSpinBox, does not work -.-
>
> --
> You received this bug notification because you are a member of Mixxx
> Development Team, which is subscribed to Mixxx.
> https://bugs.launchpad.net/bugs/992811
>
> Title:
> BPM Lock Icon
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/mixxx/+bug/992811/+subscriptions
>

Max Linke (max-linke) wrote :

Ah ok. I've tried the other skins now as well. The alternating colors are only missing in Outline.

Phoney is the only other skin that makes problems. The delegate does not show it correctly if a line is selected

Max Linke (max-linke) wrote :

I think I have the styling now covered. LateNight 1280x800 shows an example implementation of how it is possible to style the QSpinBox. There are placeholder *.png's for all the new buttons (lock Icon, up/down button in the spinbox).

To merge this with the 1.11.x branch it is required to add adopted stylings for all the skins since they all have slightly different backgrounds. I will wait for the merge until jus says he has some graphics for the lock Icon and is happy with the styling of the spinbox

Max Linke (max-linke) on 2013-05-03
Changed in mixxx:
status: In Progress → Fix Committed
jus (jus) wrote :

Meh, this is how it currently looks on OSX.
Also the up/down control is missing.
QSpinbox is just awful to style so it works on all platforms, we had the opportunity already with the Transition-time spinbox in AutoD-J

Max Linke (max-linke) wrote :

Hm that is strange. everything look fine under linux/windows. On Windows the selected row color changes to white in edit mode though. We can to the same styling that we did in the Auto-DJ Also their is not real alternative to QSpinbox for a editor like this.

If you want to try and style this spinbox you have to use QDoubleSpinbox#LibraryBPMSpinBox

RJ Skerry-Ryan (rryan) on 2013-05-10
Changed in mixxx:
milestone: 1.11.1 → 1.12.0
RJ Skerry-Ryan (rryan) on 2015-12-30
Changed in mixxx:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers