flathub / com.calibre_ebook.calibre

https://flathub.org/apps/details/com.calibre_ebook.calibre
14 stars 14 forks source link

Make calibre use the system theme #34

Open cognition9144 opened 4 years ago

cognition9144 commented 4 years ago

As explained in #2, calibre doesn't really use the system theme but use the built-in one which is not good-looking.

There are definitely differences. Below are examples. The upper ones respect system's Adwaita theme whereas the lower ones use the built-in theme.

Main window

calibre_use_system_theme_1 Calibre without system theme  - main window

Edit metadata

calibre_use_system_theme_2 Calibre without system theme  - edit metadata

It's trivial to add CALIBRE_USE_SYSTEM_THEME=true env variable but makes Calibre look much better.

TingPing commented 4 years ago

Generally flatpaks respect upstream choices and considering Calibre hides this behind an env var defaulting off I think they want this behavior.

cognition9144 commented 4 years ago

Generally flatpaks respect upstream choices and considering Calibre hides this behind an env var defaulting off I think they want this behavior.

Actually it's not what they want but because of bugs it introduces. With Flatpak I think it's not a problem.

Erick555 commented 4 years ago

What bugs and why they aren't problem in flatpak? If neither upstream or any distro default to system theme then why flatpak should give different experience for users

cognition9144 commented 4 years ago

What bugs and why they aren't problem in flatpak? If neither upstream or any distro default to system theme then why flatpak should give different experience for users

Flatpak from my understanding fixes the runtime environment which address the problem of "Qt 5 GTK plugin was causing crashes on many systems I tested with, so it is left out of the calibre bundle," as @kovidgoyal said (it would be better if he may confirm this). So we just need a working plugin.

But it could simpler. Actually if adwaita-qt is packaged into a Flatpak theme, it doesn't need any style plugin, since it provides Qt style directly. So no problem for that (and it's tested).

If you think it's reasonable, we may think of making it happen.

kovidgoyal commented 4 years ago

gtk themes constantly break backwards compatibility and cause hard to debug crashes/misbehavior. I dont have the time/patience to track those down. As such calibre is not going to enable the system theme by default.

cognition9144 commented 4 years ago

gtk themes constantly break backwards compatibility and cause hard to debug crashes/misbehavior. I dont have the time/patience to track those down. As such calibre is not going to enable the system theme by default.

I understand that and appretiate your work. But it doesn't seem to be any backwards compatibility issue in flatpak world? AFAIK flatpak is built to address this problem. All we need is to specify what dependencies (including versions) you decide to rely on. No need to support old or third-party libraries or configurations.

cognition9144 commented 4 years ago

Also, it won't and shouldn't give your additional workload as most things happens here.

kovidgoyal commented 4 years ago

Well if you are willing to guarantee compatibility and debug any resulting crashes yourself, feel free to enable it in the flatpak distro. I am not going to complain.

On Tue, Mar 03, 2020 at 07:21:13AM -0800, xcffl wrote:

gtk themes constantly break backwards compatibility and cause hard to debug crashes/misbehavior. I dont have the time/patience to track those down. As such calibre is not going to enable the system theme by default.

I understand that and appretiate your work. But it doesn't seem to be any backwards compatibility issue in flatpak world? AFAIK flatpak is built to address this problem.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/flathub/com.calibre_ebook.calibre/issues/34#issuecomment-594005744

--


Dr. Kovid Goyal https://www.kovidgoyal.net https://calibre-ebook.com


cognition9144 commented 4 years ago

Well if you are willing to guarantee compatibility and debug any resulting crashes yourself, feel free to enable it in the flatpak distro. I am not going to complain.

That would be great!

Erick555 commented 4 years ago

Well, currently calibre flatpak uses official upstream binaries and it's not possible to fix issues like this one without upstream help. For this reason I think losing upstream support only because someone found it too hard to set one env variable themselves which they have to set in every other platform anyway isn't a wise choice.

cognition9144 commented 4 years ago

I appreciate you and the upstream's work very much. I think he only talks about the bug introduced by that option. This can be determined by just turning off that option. Also I don't find it hard to do so. Just thinking about making the experience for other users more pleasant.

BTW the adwaita kstyle is already there as org.kde.KStyle.Adwaita, so it seems not so hard to implement this feature.

cognition9144 commented 4 years ago

Well, so simply using org.kde.Platform as runtime and --env=CALIBRE_USE_SYSTEM_THEME=truemake it. It's easy to debug and at least the upstream will not be unhappy about switching to org.kde.Platform.

Erick555 commented 4 years ago

kde platform is ~228MB bigger than freedesktop platform and all additional content is useless for calibre which uses their own qt so this is another major obstacle.

Also what if user uses different theme than adwaita?

ghisvail commented 4 years ago

kde platform is ~228MB bigger than freedesktop platform and all additional content is useless for calibre which uses their own qt so this is another major obstacle.

Imo, the improved ethetics do not outweigh the cost of the increased size of the runtime.

Rather, I believe efforts should be spent on a proper source build (#17), which may provide better opportunities for integration with other themes.

cognition9144 commented 4 years ago

kde platform is ~228MB bigger than freedesktop platform and all additional content is useless for calibre which uses their own qt so this is another major obstacle.

Well, I think in most cases people install all three SDKs. However if they don't, I agree with you they may need that 200+MB (interestingly we are talking about 200MB+ out of 800MB+, but still worth to think of a better solution). But I notice that a lot of Flatpaks are built against freedesktop SDK, which means probably they will not get native experience (meaning they will look wired whether you use KDE or Gnome)

Also what if user uses different theme than adwaita?

After some investigation, it seems that, under GNOME:

cognition9144 commented 4 years ago

Rather, I believe efforts should be spent on a proper source build (#17), which may provide better opportunities for integration with other themes.

The discussion there even inspires that we may bundle org.kde.KStyle.Adwaita (for GNOME users) and org.gtk.Gtk3theme.Breeze (for KDE users) directly into Calibre Flatpak, at the cost of 394.2 kB + 18.0 MB.

According to the documentation, the theme files are even deduplicated. So it might be a good approach to be applied by other projects?

Erick555 commented 4 years ago

But I notice that a lot of Flatpaks are built against freedesktop SDK, which means probably they will not get native experience (meaning they will look wired whether you use KDE or Gnome)

On KDE all my installed flatpaks that use freedesktop SDK runtime (some of them use gtk toolkit and some qt one) are native looking and indistinguishable from non-flatpak apps. This includes calibre with CALIBRE_USE_SYSTEM_THEME=true set. I don't know how it looks in other DE though.

cognition9144 commented 4 years ago

On KDE all my installed flatpaks that use freedesktop SDK runtime (some of them use gtk toolkit and some qt one) are native looking and indistinguishable from non-flatpak apps. This includes calibre with CALIBRE_USE_SYSTEM_THEME=true set. I don't know how it looks in other DE though.

Perhaps you've installed both the kde & gnome runtime?

cognition9144 commented 4 years ago

But I notice that a lot of Flatpaks are built against freedesktop SDK, which means probably they will not get native experience (meaning they will look wired whether you use KDE or Gnome)

On KDE all my installed flatpaks that use freedesktop SDK runtime (some of them use gtk toolkit and some qt one) are native looking and indistinguishable from non-flatpak apps. This includes calibre with CALIBRE_USE_SYSTEM_THEME=true set. I don't know how it looks in other DE though.

Suprising I can't really find an app built upon Qt but only uses freedesktop SDK... But that makes sense, unless they are prebuilt or add Qt as their module during the building process.

Or may you list any app that isn't in the above cases if there is any?

Erick555 commented 4 years ago

Perhaps you've installed both the kde & gnome runtime?

I have kde runtime (not gnome) installed but thy it would matter for apps which don't use it?

Suprising I can't really find an app built upon Qt but only uses freedesktop SDK... But that makes sense, unless they are prebuilt or add Qt as their module during the building process.

I have installed two. One is calibre, second one is https://github.com/flathub/com.elsevier.MendeleyDesktop . Both are prebuilt apps for whom additional qt in runtime may cause incompatibility conflicts and brings no benefits.

cognition9144 commented 4 years ago

I have kde runtime (not gnome) installed but thy it would matter for apps which don't use it?

That is for those who use kde/gnome runtime. They actually use the corresponding theme packages (runtime extensions).

I have installed two. One is calibre, second one is https://github.com/flathub/com.elsevier.MendeleyDesktop . Both are prebuilt apps for whom additional qt in runtime may cause incompatibility conflicts and brings no benefits.

As expected Mendeley looks so legacy on GNOME. I will test what it looks like on KDE.

So if #17 passes, there will not be any trade-off regarding additional size or incompatibility, since we will have qt binary during the building process (and that's all we need). There is no freedesktop sdk based GTK apps that don't look native on KDE.

Maybe we can do the native theming case-by-case. If an app is stable to have native theming, we make it looks pleasant. Vice versa.

Additionally, to clarify, I don't believe outside environment will have such big impact to make apps running in the same containerized environment show divergent behaviors. Therefore, if it's well-tested when packaging and have no error, we should expect it will be OK on end-user's computer.

TingPing commented 4 years ago

The discussion there even inspires that we may bundle org.kde.KStyle.Adwaita (for GNOME users) and org.gtk.Gtk3theme.Breeze (for KDE users) directly into Calibre Flatpak, at the cost of 394.2 kB + 18.0 MB.

One is a Qt theme, one is a GTK theme. You don't need both. (Qt doesn't load GTK3 themes in any form to my knowledge)

cognition9144 commented 4 years ago

The discussion there even inspires that we may bundle org.kde.KStyle.Adwaita (for GNOME users) and org.gtk.Gtk3theme.Breeze (for KDE users) directly into Calibre Flatpak, at the cost of 394.2 kB + 18.0 MB.

One is a Qt theme, one is a GTK theme. You don't need both. (Qt doesn't load GTK3 themes in any form to my knowledge)

That's true. So it'll be extra 18MB for Qt apps and 393.2 kB for GTK+ apps.

LinAGKar commented 4 years ago

kde platform is ~228MB bigger than freedesktop platform and all additional content is useless for calibre which uses their own qt so this is another major obstacle.

It's shouldn't be bundling its own Qt in the first place, that's the whole point of the runtime system.

The discussion there even inspires that we may bundle org.kde.KStyle.Adwaita (for GNOME users) and org.gtk.Gtk3theme.Breeze (for KDE users) directly into Calibre Flatpak, at the cost of 394.2 kB + 18.0 MB.

That seems like a hack that will break if you don't one of those specific themes. Also, would that use the color scheme set in the KDE settings? Because Qt apps using the KDE runtime do (I've tested with org.qgis.qgis).

IMO, it should use the KDE runtime, which would probably be installed anyway if flatpaks become heavily used, in which case it will work with system colors and kstyle themes.

Erick555 commented 4 years ago

It's shouldn't be bundling its own Qt in the first place, that's the whole point of the runtime system.

The fact is it does and will do in foreseeable future, see https://github.com/flathub/com.calibre_ebook.calibre/issues/17

That seems like a hack that will break if you don't one of those specific themes. Also, would that use the color scheme set in the KDE settings? Because Qt apps using the KDE runtime do (I've tested with org.qgis.qgis).

If you use theme that isn't available in flatpak then it will never work there. Custom color scheme works right now in calibre flatpak.

IMO, it should use the KDE runtime, which would probably be installed anyway if flatpaks become heavily used, in which case it will work with system colors and kstyle themes.

For the reasons stated above it's pointless right now and may be harmful if there is a mismatch between runtime qt and calibre own qt versions.

LinAGKar commented 4 years ago

The fact is it does and will do in foreseeable future, see #17

It relying on Qt internals is its own problem, but OK, that's not gonna be solved here.

If you use theme that isn't available in flatpak then it will never work there.

Though it will if the theme is packaged for flatpak, which is not the case the if the app only uses themes bundled with it.

Custom color scheme works right now in calibre flatpak.

Not in my experience. I can change the color scheme in the KDE settings, and that changes qgis, but not calibre.

Erick555 commented 4 years ago

Though it will if the theme is packaged for flatpak, which is not the case the app only uses themes bundled with it.

Yes, it must be packaged for flatpak so you can't use any theme but I think there is only one QT theme available right now (org.kde.KStyle.Adwaita)

Not in my experience. I can change the color scheme in the KDE settings, and that changes qgis, but not calibre.

Did you set CALIBRE_USE_SYSTEM_THEME=true variable?

LinAGKar commented 4 years ago

Did you set CALIBRE_USE_SYSTEM_THEME=true variable?

Yes, I tried with flatpak override --env=CALIBRE_USE_SYSTEM_THEME=true com.calibre_ebook.calibre and CALIBRE_USE_SYSTEM_THEME=true flatpak run com.calibre_ebook.calibre, but it still does not respect the system color scheme.

Erick555 commented 4 years ago

Is this on kde?

LinAGKar commented 4 years ago

Is this on kde?

Yes, it is, on openSUSE Tumbleweed. I set it to the Breeze Dark color scheme in the KDE color settings, and that works for calibre from RPM, and the qgis flatpak, but not for the calibre flatpak.

Erick555 commented 4 years ago

I don't know then, for me it just works in Arch Linux, kde.

alxlg commented 4 years ago

Did you set CALIBRE_USE_SYSTEM_THEME=true variable?

Yes, I tried with flatpak override --env=CALIBRE_USE_SYSTEM_THEME=true com.calibre_ebook.calibre and CALIBRE_USE_SYSTEM_THEME=true flatpak run com.calibre_ebook.calibre, but it still does not respect the system color scheme.

Same here. KDE Neon, org.kde.Platform installed, QtWidget theme set to Breeze. It works with VLC, but not with Calibre.

@Erick555 Do you have any other Flatpak package installed?

Erick555 commented 4 years ago

Yes, for me it works everywhere.

Erick555 commented 4 years ago

@LinAGKar @alxlg sorry for this taking so long but I finally found what is missing: you may try running flatpak run --filesystem=xdg-config/kdeglobals:ro com.calibre_ebook.calibre and this should make color schemes work (at least on kde plasma session and assuming it works for other flatpaks for you)

LinAGKar commented 4 years ago

That does work, though only that's not a permanent solution.

Erick555 commented 4 years ago

@LinAGKar this will be permanent solution when https://github.com/flathub/com.calibre_ebook.calibre/pull/59 is merged

ghisvail commented 3 years ago

@Erick555 @LinAGKar is there anything left actionable here? Otherwise, let's close it.

Erick555 commented 3 years ago

There was a request for passing CALIBRE_USE_SYSTEM_THEME=true by default but I agree with a comment that we should honor upstream choice to leave it as user opt-in.

ghisvail commented 3 years ago

we should honor upstream choice to leave it as user opt-in.

Agreed. Leaving it as wontfix then.

cognition9144 commented 3 years ago

So, for anyone who want to do this by their own, you may run flatpak run --env=CALIBRE_USE_SYSTEM_THEME=true --runtime=org.kde.Platform --runtime-version=5.15 com.calibre_ebook.calibre. And you may even save the override by flatpak override --env=CALIBRE_USE_SYSTEM_THEME=true. Unfortunately there is no way to override the runtime permanently

ghisvail commented 3 years ago

Thanks for your input 👍

Le mar. 9 févr. 2021 à 16:47, xcffl notifications@github.com a écrit :

So, for anyone who want to do this by their own, you may run flatpak run --env=CALIBRE_USE_SYSTEM_THEME=true --runtime=org.kde.Platform --runtime-version=5.15 com.calibre_ebook.calibre. And you may even save the override by flatpak override --env=CALIBRE_USE_SYSTEM_THEME=true. Unfortunately there is no way to override the runtime permanently

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/flathub/com.calibre_ebook.calibre/issues/34#issuecomment-776037458, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAO7U36NI6DU54MEHJOHBG3S6FKKLANCNFSM4LAA357A .

cognition9144 commented 1 year ago

Similar to https://github.com/flathub/org.kde.WaylandDecoration.QGnomePlatform-decoration/issues/4, it's no longer possible to use the system theme, without Calibre built against the runtime or the runtime to include the qt_resourceFeatureZstd symbol.

When running QT_DEBUG_PLUGINS=1 calibre, it reports

qt.core.library: "/usr/share/runtime/lib/plugins/Adwaita/styles/adwaita.so" cannot load: Cannot load library /usr/share/runtime/lib/plugins/Adwaita/styles/adwaita.so: (/usr/share/runtime/lib/plugins/Adwaita/lib/libadwaitaqt6.so.1: undefined symbol: qt_resourceFeatureZstd, version Qt_6)
qt.core.plugin.loader: QLibraryPrivate::loadPlugin failed on "/usr/share/runtime/lib/plugins/Adwaita/styles/adwaita.so" : "Cannot load library /usr/share/runtime/lib/plugins/Adwaita/styles/adwaita.so: (/usr/share/runtime/lib/plugins/Adwaita/lib/libadwaitaqt6.so.1: undefined symbol: qt_resourceFeatureZstd, version Qt_6)"
QApplication: invalid style override 'Adwaita-HighContrastInverse' passed, ignoring it.
    Available styles: HighContrastInverse, HighContrast, Adwaita-HighContrastInverse, Adwaita-HighContrast, Adwaita-Dark, Adwaita, Windows, Fusion
qt.core.plugin.factoryloader: checking directory path "/usr/share/runtime/lib/plugins/iconengines" ...
qt.core.plugin.factoryloader: checking directory path "/app/lib/calibre/plugins/iconengines" ...
qt.core.plugin.factoryloader: looking at "/app/lib/calibre/plugins/iconengines/libqsvgicon.so"

which makes it fallback to the Fusion theme.

RokeJulianLockhart commented 1 year ago

@xcffl, the issue that https://github.com/flathub/org.kde.WaylandDecoration.QGnomePlatform-decoration/issues/4 depends upon was purportedly remediated by https://github.com/DavidoTek/ProtonUp-Qt/issues/98#event-7376680589, so does that not remediate this?

cognition9144 commented 1 year ago

@xcffl, the issue that flathub/org.kde.WaylandDecoration.QGnomePlatform-decoration#4 depends upon was purportedly remediated by DavidoTek/ProtonUp-Qt#98 (comment), so does that not remediate this?

Calibre compiles against a patched version of Qt. So I don't think it's appropriate to remove all the static libraries.

Rather, there should be a fix in the runtime to include this symbol.

Erick555 commented 1 year ago

which kde runtime you test, 6.3?

cognition9144 commented 1 year ago

which kde runtime you test, 6.3?

6.4. 6.3 raises

Failed to import PyQt module: PyQt6.QtWidgets with error: /usr/lib/x86_64-linux-gnu/libQt6Core.so.6: version `Qt_6.4' not found (required by /app/lib/calibre/lib/calibre-extensions/PyQt6.QtWidgets.so)
cognition9144 commented 1 year ago

@xcffl, the issue that flathub/org.kde.WaylandDecoration.QGnomePlatform-decoration#4 depends upon was purportedly remediated by DavidoTek/ProtonUp-Qt#98 (comment), so does that not remediate this?

Well, I tried with

      - rm /app/lib/calibre/lib/libQt6*

and yes, it works :)

cognition9144 commented 1 year ago

@Erick555 the only Qt patch I can find and used by Calibre is this one: https://github.com/kovidgoyal/bypy/blob/master/patches/qtbug-110070.patch. So somehow it's safe to let it use the runtime provided libraries.

Erick555 commented 1 year ago

The amount of patches will change over time. The qt version calibre uses will change over time. Calibre supports only Qt libraries provided on their own.

cognition9144 commented 1 year ago

The amount of patches will change over time. The qt version calibre uses will change over time. Calibre supports only Qt libraries provided on their own.

Matching the Qt version and applying patches is exactly the job of packagers. I'm not pushing you to support it. I just want to make it clear that currently only 1 insignificant patch is needed and Qt version matching is a mandatory task for most Qt-based applications on flathub.org.

For the specific problem I mentioned, maybe we can address it at the SDK level. Still investigating.