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

Inapropriate icon on snap #2568

Closed
vozdeckyl opened this issue Apr 27, 2022 · 17 comments · Fixed by #2616
Closed

Inapropriate icon on snap #2568

vozdeckyl opened this issue Apr 27, 2022 · 17 comments · Fixed by #2616
Labels
Bug It's a bug Snap Issues related to snap

Comments

@vozdeckyl
Copy link
Contributor

Flameshot Version

Flameshot v11.0.0 (03af9d8)
Compiled with Qt 5.12.8

Installation Type

Compiled from source

Operating System type and version

Linux

Description

After saving the screenshot, the information bar that pops up seems to have an inapropriate (missing?) icon.

Steps to reproduce

  1. run ./flameshot gui
  2. select area
  3. save screenshot to desktop

Screenshots or screen recordings

bug

System Information

  1. Ubuntu 20.04.4 x86
  2. resolution 1920x1080
  3. X11
@vozdeckyl vozdeckyl added the Unconfirmed Bug The bug is not confirmed by anyone else. label Apr 27, 2022
@vozdeckyl
Copy link
Contributor Author

If this argument is replaced with an absolate path to the icon, it will work.

<< "flameshot" // icon

@mmahmoudian
Copy link
Member

mmahmoudian commented May 1, 2022

As far as I know, it should not be replaced with absolute path as many use custom icons, for instance for dark mode.

I have these on my system:

image

I think for some reason you don't have icon packs. I don't know if this is something that can/should be solved by Flameshot. Perhaps @borgmanJeremy and @veracioux have a solution to this.

@borgmanJeremy
Copy link
Contributor

Yes I agree with @mmahmoudian especially because you have compiled from source but not run "make install" or installed any other way to the system. The notification is handled by the system, not flameshot, so if the icon is not installed in the notification daemon search path it will not be found.

@vozdeckyl
Copy link
Contributor Author

Hi @borgmanJeremy,
I thought that running flameshot that was compiled from a source could be the issue, so I tried installing it via Snap and it has the same issue:

$ flameshot --version
Flameshot v11.0.0 ()
Compiled with Qt 5.15.3

However, when installing the older version via apt it the icon is there and it works fine:

$ flameshot --version
Flameshot 0.6.0+git20191001-2(Debian)
Compiled with Qt 5.12.5

Maybe there is something wrong with the snap?

@vozdeckyl
Copy link
Contributor Author

Hi @borgmanJeremy, @mmahmoudian,
So to me this looks like snap doesn't install the icons correctly:

  1. sudo apt install flameshot
  2. verify icons installed:
$ find /usr/share/icons -name flameshot.* -type f
/usr/share/icons/hicolor/48x48/apps/flameshot.png
/usr/share/icons/hicolor/128x128/apps/flameshot.png
/usr/share/icons/hicolor/scalable/apps/flameshot.svg
  1. and they work:
$ notify-send -u normal "TEST" "Test" -i flameshot
  1. sudo apt remove flameshot

However, with snap:

  1. snap install flameshot
  2. icons are missing:
find /usr/share/icons -name flameshot.* -type f
  1. and also don't work
notify-send -u normal "TEST" "Test" -i flameshot

Should I create a separate issue for this? Thanks.

@mmahmoudian mmahmoudian added the Snap Issues related to snap label May 1, 2022
@mmahmoudian
Copy link
Member

Seems like compelling evidence. Thank you. I'll reopen this and assign it to myself to see if I can reproduce it in a clean VM.

@mmahmoudian mmahmoudian reopened this May 1, 2022
@mmahmoudian mmahmoudian self-assigned this May 1, 2022
@borgmanJeremy borgmanJeremy changed the title Inapropriate icon Inapropriate icon on snap May 3, 2022
@borgmanJeremy borgmanJeremy added Bug It's a bug and removed Unconfirmed Bug The bug is not confirmed by anyone else. labels May 3, 2022
@borgmanJeremy
Copy link
Contributor

So snap's CANNOT install the icons outside the confinement, thats the whole purpose of a snap. After some research I'm not sure this is fixable without reworking the notification system to not use d-bus. I'm likely to close this as "won't fix" unless someone can turn up a way to fix resolve this.

@mmahmoudian mmahmoudian removed their assignment May 3, 2022
@mmahmoudian
Copy link
Member

@borgmanJeremy how about keeping it open but in backlog in case someone want to work on it?

@borgmanJeremy
Copy link
Contributor

My understanding is it is not fixable without reworking our notifications completely.

@vozdeckyl
Copy link
Contributor Author

@borgmanJeremy What about using snap layouts? They can be used to map system folders into the snap folders. Maybe if we map /usr/share/icons/.. into $SNAP/usr/share/icons/.. somehow, it might work? Let me investigate.

@borgmanJeremy
Copy link
Contributor

I think we need to do the opposite. Map the icon from inside the snap to the system.

@vozdeckyl
Copy link
Contributor Author

Yeah, you are right. But I think that's exactly what layouts do as that's a very common issue. (I probably should've phrased it the other way around)

@vozdeckyl
Copy link
Contributor Author

Hi @borgmanJeremy,
I looked into this and you were right: this won't work. The problem is that it's not the application accessing the icon, but the operating system, which gets a message from the application. If it were the app trying to access the icon in /usr/share/icons/..., the snap layouts could map this into $SNAP/usr/share/icons/.., where the icon could be saved and it would work ok. However, since it's the system and it expects the icon to be in /usr/share/icons/..., we'd need to move the icon there, which I believe is not possible in the snap installation as it doesn't have writing permission for system folders.

The only quick work-around I could think of is to replace flameshot in the system notification by the absolute path to $SNAP/usr/share/icons/../flameshot.svg if the application is run via snap. This is something I mentioned above, but I'm not sure if this solution would be ideal.

@borgmanJeremy
Copy link
Contributor

What if we make it a compile time string and pass in the alternate value in the snapcraft CI only

@vozdeckyl
Copy link
Contributor Author

Hi @borgmanJeremy,
Yes, that would be even better! Let me tackle this.

@borgmanJeremy
Copy link
Contributor

Will do, let me know if you hit any roadblocks. Always happy to have more contributors :)

@vozdeckyl
Copy link
Contributor Author

I tested it and it seems to be working. When testing snapcraft locally, don't forget to change source in snapcraft.yaml from the github link to ".", otherwise it will just pull the master branch during the build. I spent quite a while on this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug It's a bug Snap Issues related to snap
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants