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.

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

Use YQUI icon loader in YQImage whenever possible #100

Merged
merged 6 commits into from Feb 28, 2019
Merged

Use YQUI icon loader in YQImage whenever possible #100

merged 6 commits into from Feb 28, 2019

Conversation

shundhammer
Copy link
Contributor

@shundhammer shundhammer commented Feb 27, 2019

Trello

https://trello.com/c/5fgf38AA/

Bugzilla

https://bugzilla.suse.com/show_bug.cgi?id=1119688
https://bugzilla.suse.com/show_bug.cgi?id=1122174

Problem

Header icons for all the views of the partitioner were gone after the migration to theme icons and SVGs.

Cause and Fix

The YQImage had used an inconsistent approach to load the icons: If they could not be found in the desktop's theme, it would prepend :/ and append .svg to try to load them from the compiled-in Qt resource files, resulting in a path :/myicon.svg.

But that was wrong: It should have been either :/myicon or :/icons/myicon.svg. All icons in that compiled-in resource file are in a subdirectory icons/ with the .svg suffix, and there are aliases in the icon description file for just the plain name (without the .svg suffix).

Worse, it was a lot of code duplication; the reference how to do that is now the new YQUI::loadIcon() method. This is now used unless the image is specified with an absolute path (i.e. starting (!) with a / slash; but not :/).

This results in less code and more robust code.

Screenshots

(Notice the icons at the page headers)

partitioner-system-view
partitioner-hard-disks-view
partitioner-volume-management-view
partitioner-nfs-view

@ancorgs
Copy link

ancorgs commented Feb 27, 2019

Does this fix also applies to these other missing icons in the "configure" button? http://paste.opensuse.org/view//97095243

@shundhammer
Copy link
Contributor Author

shundhammer commented Feb 27, 2019

Does this fix also applies to these other missing icons in the "configure" button? http://paste.opensuse.org/view//97095243

Pretty sure no. This is only for the YQImage widget. That other thing is a menu button.

But the problem might be the same there, and the fix should be even easier: Just use YQUI::loadIcon() consistently and not let that widget do its own thing.

I just checked: it does use YQUI::loadIcon(). So maybe the icon names are just wrong (using a suffix or a path?).

Copy link
Member

@lslezak lslezak left a comment

Choose a reason for hiding this comment

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

LGTM

I do not know what's wrong with Travis, I can build the package locally, but build in Docker for some strange reason fails. You can merge it even with failing Travis.

@shundhammer shundhammer merged commit e849ec0 into libyui:master Feb 28, 2019
@shundhammer shundhammer deleted the huha-partitioner-icons branch February 28, 2019 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants