Allow skins to define new library sidebar icons

[Lee Matos]

Self explanatory. It'd be nice if Mixxx checked the skin folder for substitutions and if not, fall back to the default icons. I'd be happy to try and take a stab at this one.

[Daniel Schürmann (daschuer)]

+1

It could also be nice to allow to set the icon color by skin and respect color schems.
We have already the facility to load and recolor bitmaps in Mixxx.

@Lee: Do you have time and fun to adopt this bug?

[Lee Matos (lbot)]

Yeah, I'll give it a shot.

[Daniel Schürmann (daschuer)]

Cool thank you.

Do you know where to start? How will the skin interface look like?
Don't forget to ask for help if you need any!

[Lee Matos (lbot)]

On the skin side I was thinking just have the files be in the root and if they are present to load that version instead of the default. I think that's a good simple start. I could use some help on https://bugs.launchpad.net/mixxx/+bug/1325249 :P

[Lee Matos (lbot)]

It looks like each feature defines getIcon(). It seems like that's the best place for each conditional:

https://github.com/mixxxdj/mixxx/blob/4845e95acb542120191f309e050d785699730048/src/library/recording/recordingfeature.cpp#L32-L34

I thought that I saw an example of conditional image rendering somewhere in the codebase but it eludes me at the moment. Know of any?

[Lee Matos (lbot)]

I'm wondering if getIcon() should be abstracted a level up so that it doesn't have to be defined for each feature but instead you pass in the icon name and it works that way:

Psuedo-code
------------------

class feature:

method getIcon(icon_name):
  do stuff

recording_feature = new feature();

recording_feature.getIcon("recording_icon.svg");

analysis_feature = new feature();

analysis_feature.getIcon("analysis_icon.svg");

etc.

Seems like a cleaner solution I'm just not sure if there is a reason that stopping it from being done that way. Thoughts?

[Daniel Schürmann (daschuer)]

> I thought that I saw an example of conditional image rendering somewhere in the codebase but it eludes me at the moment. Know of any?

Probably this:
https://github.com/mixxxdj/mixxx/blob/0d89ecbdaaa1a785665fe5791222fa3ef8dd4467/src/widget/wpixmapstore.cpp

----

> I'm wondering if getIcon() should be abstracted a level up ...

Yes, good idea. But the Icon name should be still set inside the specific feature (child class)

What about a
LibraryFeature::getIcon()

And a virtual RecordingFeature::getIconName()

Pseudo code:
LibraryFeature::getIcon() {
    Name = getIconName()
    icon = Look up in skin
    if nothing found {
         icon = Lookup from resources
    }

[Daniel Schürmann (daschuer)]

For the second step:
            Check for <IconColor> in skin
            recolour Icon
           Apply color schema if any.

see https://github.com/mixxxdj/mixxx/blob/4a9f5b24bac3bdfcfe9c58d428df62a002a8312a/src/skin/colorschemeparser.cpp

[Lee Matos (lbot)]

I read in an older bug (can't find it ATM) that rryan was considering dropping color scheme support. (Which I agree with) Do we know if that's going to happen $soon? I'll continue working on the first portion and keep the second (admittedly more complex) portion up in the air for now.

[Daniel Schürmann (daschuer)]

Hi Lee,

A less complex fix, like you are going to do is already a great improvement. Thank you for all your work.

---

I could remember a discussion like that as well.

I think it was discussed when the color scheme where suffering:
https://bugs.launchpad.net/mixxx/+bug/1094785
https://bugs.launchpad.net/mixxx/+bug/1071500

Today we have re-sizable skins and a working color scheme support, so lets re-think the issue.

IMHO the color scheme mechanic could become useful beside of offering a dark and light skin variant, if we allow to
set <Variable name="???"/> or COs by the color scheme.

This way we have full control to replace images or tweak the skin for Mixing styles, without make a new skin.

Thats why I have started to add color schemes to Shade skin. https://github.com/mixxxdj/mixxx/pull/258

Kind regards,

Daniel

[Lee Matos (lbot)]

Daniel,

I'm having trouble getting access to the config for SkinLoader in libraryfeature. Hopefully this gist will help to explain:

https://gist.github.com/leematos/a7a66ef2c1a3397f00ca

I'm so close!

[Lee Matos (lbot)]

This is what I ended up committing tonight. This compiles but segfaults :/ https://github.com/leematos/mixxx/commit/7fcbaa19ee365282ae32c6d97b0c0daefdeaab90

Any tips or help appreciated!

-Lee M

[Daniel Schürmann (daschuer)]

Where does the segfault happen?
You may provide a backtrace.

gdb ./mixxx

> run
~ segfault here
> bt

----

I would prefer not to calculate the current skin over and over again in each Library feature for each getIcon() call
What about piping it thought
src/skin/legacyskinparser.cpp
Library::bindSidebarWidget
to SidebarModel

You may use
QIcon getIcon(QString skinPath)
then or move it to SidebarModel

Do you have a clue, how often getIcon() is called?
If it is called very often, you may consider to cache the Icon somewhere.

[Lee Matos (lbot)]

This is what I got:

http://pastebin.com/a5BuA9dx

I'm not against piping it through skinparser. That makes sense. I'm not sure how often getIcon is called. How would I go about profiling that?

-Lee M

[Daniel Schürmann (daschuer)]

Just place a
qDebug() << "++++++++++ getIcon()";
into get icon and look at the console output.

In other cases you may use
https://github.com/mixxxdj/mixxx/blob/master/src/util/counter.h

IMHO this piping is not a performance issue at all. It just feels odd from the object orientated point of view.
This might be a source of an error later.

[Daniel Schürmann (daschuer)]

You are suffering a Null pointer access

frame #4: 0x00000001000461e4 mixxx`ConfigObject<ConfigValue>::get(this=0x0000000000000000, k=ConfigKey at 0x00007fff5fbfbba0) + 20 at configobject.h:152

QString configSkin = m_pConfig->getValueString(ConfigKey("[Config]","Skin"));

This should be solved by the piping as well.

[Lee Matos (lbot)]

Hey Daniel,

Somehow I missed this comment and ended up coming to the conclusion that this line was the problem. I'm not sure how to pipe to figure out the null pointer. I assume I'm redefining m_pConfig which is leading the the skin path config option not being properly set.

I tried to just deference m_pConfig but that threw an error. It's strange because I can get the resource path from m_pConfig (qDebug() << m_pConfig->getResourcePath()) in debug output inside of that function.

I'm also a little confused about m_pConfig, pConfig and the m_ prefixes in mixxx. Can you share some insight? Earlier you mentioned "IMHO this piping is not a performance issue at all." were you talking about the inheritance of getIcon?

[Daniel Schürmann (daschuer)]

Oh, I have just noticed, i have commented the wrong Bug:

Here my post from https://bugs.launchpad.net/mixxx/+bug/1338014:

Hi Lee,

I have just compiled your library-icons branch and it works fine without a Segfault at Ubuntu Saucy.
It seams that getIcon is called quite often.
Most Icons are vanished thought.

Maybe you segfault was bug https://bugs.launchpad.net/bugs/1338014

By the way: Once it is finished I like to use it in the Shade skin.

Hope that helps,

Daniel

[Daniel Schürmann (daschuer)]

Here my repy to Lee's comments in https://bugs.launchpad.net/mixxx/+bug/1338014/comments/3:

> 1) when switching branches, how can I force a recompile? Sometimes it won't recompile if I haven't
> made any changes besides switching branches.

1) scons -c may work, but I am unsure.
The build environment tries to avoid rebuild whenever possible. If you are switching a branch, it tries to copy a old build directory from the cache folder. if it does not exist, It recompiles with the build directory from the previous branch.
Please note: If you switch the branch, recompile with "nothing to do" the mixxx binary in the project root is still th one from the last branch. But there is a copy of the current branch in the build directory.
Pending Bug: https://bugs.launchpad.net/mixxx/+bug/1197990

> 2) I'm not sure what the segfault was but I deleted the build directory and the mixxx executable and a
> full recompile fixed it.
> 3) The icons were missing because most getIconName() for features were set to "1". That's an easy fix.
> 4) You weren't getting the segfault because I commented out the line that was causing it:
> https://github.com/leematos/mixxx/blob/3290f82b735b4365b5a8681c1a94099efc765688/src/library/libraryfeature.cpp#L24
> How can I get the skinpath there? Once that happens I'll submit a pull request.

I have already tried to give some hints to a possible way in:
https://bugs.launchpad.net/mixxx/+bug/1327614/comments/13
Did you try it? Do you need more details?

[Lee Matos (lbot)]

I was a little confused about your use of "piping". If you have any other suggestions or more details they would be greatly appreciated. I'm really close to getting it working. :)

-Lee M

[Max Linke (max-linke)]

> 1) when switching branches, how can I force a recompile? Sometimes it won't recompile if I haven't

git clean -fdx

This will delete all build files and give you a clean checkout. After this scons will recompile everything

[Sean M. Pappalardo (pegasus-renegadetech)]

@lbot have you made any further progress on this?

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/7499