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

Improve support for displaying Vorta's logo in Linux notifications #609

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SergioRAgostinho
Copy link

@SergioRAgostinho SergioRAgostinho commented Sep 3, 2020

  1. Moved logo icon from packaging content to a formal asset of the application, since the notification image benefits all Linux users, regardless of the installation method;
  2. Renamed the logo to clarify its content, better fitting its new location.

A follow up on #563. A description of the problem can be found in here and it affected non-flatpack users.

Screenshot from 2020-09-03 17-21-33

- Moved logo icon from packaging content to a formal asset of the
application, since the notification image benefits all Linux users,
regardless of the installation method;
- Renamed the logo to clarify its content, better fitting its new
location.
@samuel-w
Copy link
Contributor

samuel-w commented Sep 5, 2020

This isn't working for me, bottom is this tree, top is master tree.
Using XFCE4
broken
Also has no icon in flatpak.

@SergioRAgostinho
Copy link
Author

SergioRAgostinho commented Sep 5, 2020

Let's break this in points, to figure out how to move next:

  1. Are you in agreement that this icon needs to be promoted to regular asset and not just simply live in the packaging folder? Reason: it's an icon used in Linux notifications in general and thus agnostic to the packaging method.
  2. Is the monochromatic logo being used anywhere other than for notifications? My assumption was that the flatpack logo was being served by this file.

@Hofer-Julian
Copy link
Collaborator

Let's break this in points, to figure out how to move next:

1. Are you in agreement that this icon needs to be promoted to regular asset and not just simply live in the packaging folder? Reason: it's an icon used in Linux notifications in general and thus agnostic to the packaging method.

2. Is the monochromatic logo being used anywhere other than for notifications?  My assumption was that the flatpack logo was being served by [this file](https://github.com/borgbase/vorta/blob/master/package/icon.svg).

I think the already existing implementation is the way to go.
The assets are copied to the destinations according to the freedesktop specifications.
This is already done with the flatpak, and debian, arch, ... will do the same.
There was an attempt to allow something similar with pip alone, but we decided to drop it.

@SergioRAgostinho
Copy link
Author

SergioRAgostinho commented Sep 5, 2020

I think the already existing implementation is the way to go.

You're breaking the "separation of concerns". Having access to a notification logo in Linux should be agnostic of how vorta is installed and this includes pip/setuptools based installs. This is not a pip/setuptools imposed limitation like "having an application icon in an apps menu". This last one really requires os-friendly packaging mechanisms which pip does not support.

Furthermore, you're imposing an avoidable visual difference between the development environment, i.e. when you're working with vorta like this

$ python setup.py develop

, and how it is deployed.

So I'm just not really understanding why you'd rather keep it as is, instead of trying to modify this PR to ensure we have consistent notifications icons across all installation methods in the Linux ecosystem.

@Hofer-Julian
Copy link
Collaborator

I do see your point.
If you provide something, which

  • doesn't add too much complexity
  • continues to work with flatpak&traditional packages
  • works on macOS and linux
    we will most probably merge it.

We might also want to ping @sten0, how other python GUIs are currently handling this.

@SergioRAgostinho SergioRAgostinho marked this pull request as draft September 5, 2020 13:58
@m3nu m3nu closed this Apr 17, 2021
@sten0
Copy link
Contributor

sten0 commented Apr 17, 2021

@Hofer-Julian @m3nu, sorry I missed this one (I was travelling to a wedding then). Here is what Vorta 0.7.5 using the Debian package looks like:
KDE Plasma Notifications
KDE Plasma Status   Notifications
KDE Plasma Notification popup

and it appears the primary outstanding issue is with the notification popup (third screenshot), which I think is probably supposed to look like https://community.kde.org/Plasma/Notifications

Here are the details to how I'm installing Vorta (dh_python3 is magic abstraction): https://salsa.debian.org/python-team/packages/vorta/-/blob/debian/master/debian/rules

What I find curious is the monochrome icon is used in the tray, and in the notification overview, as expected, despite the fact that I haven't installed package/icon-symbolic.svg to the Debian package; however, as noted, the popup doesn't contain the icon. Given that the Vorta icon used in the tray and notification overview is already monochrome, and that the notification popup examples given at community.kde.org have colour, I'm not sure what the purpose of icon-symbolic.svg is. Is it for a macOS special case?

@sten0
Copy link
Contributor

sten0 commented Apr 17, 2021

P.S. Outside of the Makefile, I couldn't find any reference in the source (of 0.7.5) to icon-symbolic.svg, nor com.borgbase.Vorta-symbolic.svg

@m3nu m3nu reopened this Apr 17, 2021
@m3nu
Copy link
Contributor

m3nu commented Apr 17, 2021

Would be good to solve this, if it's simple.

The said icon is in the package folder https://github.com/borgbase/vorta/blob/master/package/icon-symbolic.svg

Though another size might be required.

So the remaining task is to move the icon into vorta/assets and adjust the Linux notification code?

@m3nu
Copy link
Contributor

m3nu commented Apr 21, 2021

So what kind of icon do we need to ship to make it work in all notifications and how is it referenced?

@real-yfprojects
Copy link
Collaborator

Here are the details to how I'm installing Vorta (dh_python3 is magic abstraction): https://salsa.debian.org/python-team/packages/vorta/-/blob/debian/master/debian/rules

You aren't installing package/icon-symbolic.svg to share/icons/hicolor/symbolic/apps/com.borgbase.Vorta-symbolic.svg as the flatpack does:

vorta/Makefile

Line 64 in 4871765

install -D package/icon.svg /app/share/icons/hicolor/scalable/apps/com.borgbase.Vorta.svg

What I find curious is the monochrome icon is used in the tray, and in the notification overview, as expected, despite the fact that I haven't installed package/icon-symbolic.svg to the Debian package; however, as noted, the popup doesn't contain the icon. Given that the Vorta icon used in the tray and notification overview is already monochrome

The tray icon and such are managed by Qt which uses the hdd-o-*.png icons in the src/vorta/assets. The notifications are delivered through dbus and use the system installed symbolic icon.

and that the notification popup examples given at community.kde.org have colour, I'm not sure what the purpose of icon-symbolic.svg is. Is it for a macOS special case?

That's what I am wondering about as well.

@real-yfprojects
Copy link
Collaborator

This isn't working for me, bottom is this tree, top is master tree. Using XFCE4 broken Also has no icon in flatpak.

On XFCE 14.4 it behaves exactly the other way round. Top would be this branch and bottom master.
It also works on GNOME and KDE Plasma but on KDE Neon the resolution is way to low.

@real-yfprojects real-yfprojects force-pushed the linux-icon branch 2 times, most recently from 3d549a7 to 4871765 Compare April 14, 2022 06:59
@Hofer-Julian Hofer-Julian removed their request for review June 20, 2022 10:42
@stale
Copy link

stale bot commented Jul 19, 2023

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the wontfix label Jul 19, 2023
@sten0
Copy link
Contributor

sten0 commented Jul 19, 2023 via email

@sten0
Copy link
Contributor

sten0 commented Jul 21, 2023 via email

@real-yfprojects
Copy link
Collaborator

@sten0 You don't need to put effort into this PR. There is #1522.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants