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

Provide icon with module (boo#1109310) #765

Merged
merged 6 commits into from
Nov 29, 2018
Merged

Provide icon with module (boo#1109310) #765

merged 6 commits into from
Nov 29, 2018

Conversation

hellcp
Copy link
Contributor

@hellcp hellcp commented Nov 24, 2018

No description provided.

@coveralls
Copy link

coveralls commented Nov 24, 2018

Coverage Status

Coverage decreased (-0.04%) to 22.084% when pulling 8eef81a on hellcp:master into ee89aab on yast:master.

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.

Desktop files

@@ -0,0 +1,10 @@
[Desktop Entry]
Copy link
Member

Choose a reason for hiding this comment

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

AFAICS these new .desktop files are needed just because of the Wizard.SetDesktopTitleAndIcon("installation") call, right?

The YaST control center still reads that prints this warning and ignores the file:

Warning: Skipping file "/usr/share/applications/YaST2/installation.desktop" : missing group
attribute (X-SuSE-YaST-Group)

Adding

Hidden=true

avoids that warning and that should also help to not display it in the other software by mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did that, although I am still not happy about the requirement of having desktop files for icons. It's a neat feature, but it should be an addition to real icon loading.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, using icons should be possible even without a desktop file.

Please open a separate bug for that so we can improve that.

Categories=Settings;System;Qt;

Icon=yast-installation
Exec=
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, maybe this violates the desktop file standard: "The Exec key must contain a command line.", see https://standards.freedesktop.org/desktop-entry-spec/latest/ar01s07.html for more details.

Although it's not a real problem, some OBS checks could complain in the future. I'm not a desktop file expert, but maybe using a /bin/true dummy value here could be a good idea.

When I tried to build the package locally I got these errors:

ERROR: No sufficient Category definition: /home/abuild/rpmbuild/BUILDROOT/yast2-installation-4.1.30-1.x86_64//usr/share/applications/YaST2/installation.desktop 
Please refer to https://en.opensuse.org/openSUSE:Packaging_desktop_menu_categories
WARNING: Empty GenericName: /home/abuild/rpmbuild/BUILDROOT/yast2-installation-4.1.30-1.x86_64//usr/share/applications/YaST2/installation.desktop
ERROR: No sufficient Category definition: /home/abuild/rpmbuild/BUILDROOT/yast2-installation-4.1.30-1.x86_64//usr/share/applications/YaST2/upgrade.desktop 
Please refer to https://en.opensuse.org/openSUSE:Packaging_desktop_menu_categories
WARNING: Empty GenericName: /home/abuild/rpmbuild/BUILDROOT/yast2-installation-4.1.30-1.x86_64//usr/share/applications/YaST2/upgrade.desktop
Errors in installed desktop file detected. Please refer to http://en.opensuse.org/SUSE_Package_Conventions/RPM_Macros

For some reason this check is not executed at Travis so it does not fail, I'll check if that could be improved...


Name=Installation
GenericName=System Upgrade
Comment=Install system to the system
Copy link
Member

Choose a reason for hiding this comment

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

Ah, something like "Install a new system" would be better here, but as this is just a dummy desktop file it does not matter much...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, miss on my part, I hope to remove those desktop files as soon as I patch YaST with a way to load icons directly.

@lslezak lslezak merged commit 0f61f32 into yast:master Nov 29, 2018
@yast-bot
Copy link
Contributor

✔️ Public Jenkins job #33 successfully finished
✔️ Created OBS submit request #652547

@yast-bot
Copy link
Contributor

✔️ Internal Jenkins job #35 successfully finished
✔️ Created IBS submit request #178721

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

4 participants