0003319: Themes - MantisBT

Notes
(0018646) Zalewa (developer) 2017-10-29 14:05	The included "themes.patch" does a primitive implementation of icon theming. This currently will only work for buddies.png. Unfortunately all references to ":" when creating QPixmaps or QIcons need to be replaced. Basically, QIcon(":icons/buddies.png") becomes Theme::main.icon("icons/buddies.png"). We also still want to keep the more-default-than-default icons baked into the executable. If theme fails to load, this will prevent Doomseeker from suddenly starting with a broken UI. Unfortunately, this ruins the neatness of Qt Designer - any icon that we specify there will have to be overwritten manually in code. I tried to QDir::addSearchPath("", theme_dir), but this doesn't work when the prefix is empty. QIcon(":icons/buddies.png") will not look up the theme_dir. This means that Qt Designer should no longer be used to specify icons, because its duplicated work that will get overwritten. This also means that we lose preview and gain tedium. Furthermore, we will have to go over all places in the code where icons are applied and convert the statements there to Theme::main. Any comments on how to best approach this?

(0018653) Blzut3 (administrator) 2017-10-29 18:33	Strange that adding a search path didn't work since the documentation clearly states that it should: Quote Qt's resources support the concept of a search path list. If you then refer to a resource with : instead of :/ as the prefix, the resource will be looked up using the search path list. The search path list is empty at startup; call QDir::addSearchPath() to add paths to it. That said Qt Designer unfortunately uses ":/" instead of just ":" so unfortunately that sounds like your out of luck as far as that goes. They do however mention loading qrc binaries through QResource::registerResource which sounds like it has potential even though it would make producing icon themes slightly more complicated.

(0018655) Zalewa (developer) 2017-10-29 18:41	I cooked up a minimal example to verify this and in fact it doesn't work. Quote from Blzut3 They do however mention loading qrc binaries through QResource::registerResource which sounds like it has potential even though it would make producing icon themes slightly more complicated. Yes, I was aware of that, however for the reason you stated I figured this is unviable in the long run.

(0018656) WubTheCaptain (developer) 2017-10-29 18:47	Tagging for review (Blzut3), since a patch is included. It's a nice idea overall, but later I'd imagine asking "why did we do this?". It's a complex solution with more code added to primarily tackle a problem of legally encumbered distribution of non-free buddies.png, as elegant and iconic as the icon looks like. Let's imagine distribution of non-free "buddies.png" to be barred completely: Would this patch/feature be useful anymore after that, or does it become featuritis? Thinking more open-minded about this, I can see themes to replace baked icons may have an use because Doomseeker supports more engine plugins than Doom too. I can also see e.g. a dark theme being implemented (in example: OBS Studio has one). In regards to 0003237, the existence of non-free "buddies.png" in the source archive is still something downstream distributors have to handle with care and burden. This proposed patch does not resolve that. Breaking Qt Designer is also something I'm not a fan of thought. I'd reject this patch, but would be fine on limited scope of theming like light vs dark themes. (Kdenlive supports icon theming, but alas it also has default icons baked in and it's KDE. The UI is still broken in parts with missing icons.)

(0018662) WubTheCaptain (developer) 2017-10-29 19:45 edited on: 2017-10-29 19:45	For clarification: If the current id Software / Doom "buddies.png" becomes a non-default theme in a contrib directory (and distribution is legal), all fine for me so I could perhaps support this. See 0003320, which was my primary idea of resolving the underlying issue.

(0018664) Blzut3 (administrator) 2017-10-29 20:56	Very curious. Given how popular skinning is in Windows apps it really surprises me that Qt doesn't make that easy. Oh well I guess. I don't think the registerResource solution (assuming it works) is too much of a hurdle, but I don't know how much anyone cares to skin Doomseeker.

(0018670) AOSP (reporter) 2017-10-29 23:53	Oh yes, please. Anything to solve the bad contrast the red icons have on dark Qt system themes (ie Breeze Dark).

(0018673) WubTheCaptain (developer) 2017-10-30 01:21	Feel free to assign to yourself if you're working on this.

(0018683) WubTheCaptain (developer) 2017-10-30 06:00	Actually, do we need to do anything here? See the QT_STYLE_OVERRIDE environment variable or qt5ct. https://wiki.archlinux.org/index.php/Uniform_look_for_Qt_and_GTK_applications [^] Icon themes seem to be possible too.

Only registered users can voice their support. Click here to register, or here to log in.
Supporters:	AOSP
Opponents:	WubTheCaptain

Issue History
Date Modified	Username	Field	Change
2017-10-29 13:54	Zalewa	New Issue
2017-10-29 13:54	Zalewa	File Added: themes.patch
2017-10-29 13:55	Zalewa	Relationship added	related to 0003237
2017-10-29 14:05	Zalewa	Note Added: 0018646
2017-10-29 18:33	Blzut3	Note Added: 0018653
2017-10-29 18:41	Zalewa	Note Added: 0018655
2017-10-29 18:47	WubTheCaptain	Note Added: 0018656
2017-10-29 18:47	WubTheCaptain	Status	new => needs review
2017-10-29 18:56	WubTheCaptain	Relationship added	related to 0003320
2017-10-29 19:00	WubTheCaptain	Relationship deleted	related to 0003237
2017-10-29 19:45	WubTheCaptain	Note Added: 0018662
2017-10-29 19:45	WubTheCaptain	Note Edited: 0018662	View Revisions
2017-10-29 20:56	Blzut3	Note Added: 0018664
2017-10-29 23:53	AOSP	Note Added: 0018670
2017-10-30 01:21	WubTheCaptain	Note Added: 0018673
2017-10-30 01:21	WubTheCaptain	Status	needs review => acknowledged
2017-10-30 06:00	WubTheCaptain	Note Added: 0018683
2017-10-30 06:00	WubTheCaptain	Status	acknowledged => new
2017-11-02 13:49	WubTheCaptain	Status	new => feedback
2017-11-03 12:49	WubTheCaptain	Severity	minor => feature
2018-10-05 07:27	WubTheCaptain	Priority	normal => low