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

Work with official QT #1815

Open
ItachiSan opened this issue Mar 24, 2016 · 51 comments

Comments

Projects
None yet
@ItachiSan
Copy link

commented Mar 24, 2016

This issue is opened in order to avoid in future the usage of a patched QT version.
The general idea would be to:

  • Use system FCTIX QT plugin as many Linux distros can offer this plugin, else it can be compiled as dependency
  • Avoid patching of QT stuff or send stuff upstream, in order to have a pure environment
    I would like to have feedback from developers and in case upload some code to patch stuff.
@john-preston

This comment has been minimized.

Copy link
Member

commented Mar 24, 2016

  1. as I understand I can't use it while linking Qt statically: "Note that the QPluginLoader cannot be used if your application is statically linked against Qt. In this case, you will also have to link to plugins statically"
  2. no way to link dynamically to Qt (even if you avoid all the patches I use), because my text rendering code is based on the internal code of Qt, not the public interface provided, so I can't rely that the internal methods still exist and do the expected thing.
@YtvwlD

This comment has been minimized.

Copy link

commented Mar 26, 2016

I think the idea would be to link dynamically.
I don't really understand what's the problem here. I already have QT installed on my system. Why can't Telegram use it?

@ItachiSan

This comment has been minimized.

Copy link
Author

commented Mar 26, 2016

@john-preston can't be the Telegram Desktop code refactored to use the official text rendering methods...?

@YtvwlD because Telegram Desktop use a patched version of QT5, so the methods in the official QT5 are different to the ones present in the patched version.

@abbradar abbradar referenced this issue Mar 28, 2016

Merged

Add Telegram Desktop #14266

5 of 7 tasks complete
@jhasse

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2016

This would also fix that the file dialog doesn't use my theme:

bildschirmfoto vom 2016-06-17 10-26-49

@YtvwlD

This comment has been minimized.

Copy link

commented Jun 17, 2016

Currently, nothing uses the system theme.
spectacle d13590

@AndydeCleyre

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

@john-preston

my text rendering code is based on the internal code of Qt, not the public interface provided

What's the big advantage here? Lots of users and package maintainers would love for this to use official Qt, dynamically linked. So what is so advantageous about relying on patches and unreliable internal code that makes this worth it?

@john-preston

This comment has been minimized.

Copy link
Member

commented Jul 7, 2016

@AndydeCleyre Well, when it was first made, I was working on a fast holding in memory + rendering of big amounts of text parts with graphic emoji, so that one message history can hold tens of thousands of text messages (I've tested up to 100 000) and window resize (which triggers the resize of all the messages to count the new window height) should work without lagging and scrolling should be smooth with instant switches between different conversations with big amounts of messages (tdesktop holds all the loaded messages in memory, local database for messages is not yet supported, so it needs all that) with acceptable memory consuming. I don't know how to achieve this using just Qt public interface.

@plinnell

This comment has been minimized.

Copy link

commented Sep 7, 2016

I can tell you as a developer for a project written in Qt since 2001, the patching of Qt by an application is a very bad idea. See: http:www.scribus.net We also deal with text rendering issues and have our own canvas because of the need to manipulate objects beyond text.

As a long time Linux user (15 years) and having packaged Linux applications for more than 10, not using Distro provided Qt over time is going to be an unholy, difficult to maintain mess. What happens when the patched Qt needs a security patch ? and Qt does get them ;) You are creating your own technical debt and a relatively young project needs to avoid this especially in the early days.

It seems most of what you are trying to fix, could be fixed by pulling in fcitx-qt5-devel, which provides the input capabilities for CJK.fcitx is pretty well supported on most recent distros.

Requiring a patched Qt is a very strong barrier to get your application into Linux distros. I'm one of the Factory maintainers for openSUSE and this would more than likely be rejected to be part of the distro, especially statically linking Qt and/or having your own patched Qt. I'm pretty certain Fedora/RedHat would say the same thing. Moreover, even if you apply patches, separating them out to different platforms is a best practice.

Now enough ranting. I do hope your team considers using https://openbuildservice.org to do your Linux packaging. It is cross distro and has many automation features which can ease package builds and provides infra for distribution. It can be hooked into Jenkins and speaks native git for pulling sources to build.

@gavv

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2016

I totally agree with @plinnell. Also, patched Qt was the main reason why telegram source package was rejected by Gentoo developers. Bundled dependencies are usually very unwelcome by any distro maintainers and Linux community.

@WhyNotHugo

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2016

@plinnell @gavv I don't think there's any rejection to actually stop patching Qt.

As far as I've understood, it's just that the devs haven't been able to get things working as expected with the upstream Qt, and nobody's offered another working solution (in the form of a PR). So rather than "up for debate", I think this issue is actually "awaiting patches".

@guoyunhe

This comment has been minimized.

Copy link

commented Sep 7, 2016

@gavv Some people already submited some pull request to use existing fcitx-qt5-devel package rather than patch the whole fcitx binding. But these pull requests have never been accepted.

@john-preston

This comment has been minimized.

Copy link
Member

commented Sep 7, 2016

@guoyunhe fcitx is the small thing in the patch, it just got there because the patch was already in place.

The main problem is the text-with-emoji recounting/rendering, which uses private Qt apis.

I see no way in not using it trying to make fast-enough and not-so-memory-consuming text-with-emoji processing for tdesktop. And even if there is such way, which I don't know, it will be a huge amount of work to move the existing code base to that another way.

@AndydeCleyre

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2016

@john-preston is it relatively simple to make a branch that uses Qt's slow/mem-heavy methods? How bad is it?

@john-preston

This comment has been minimized.

Copy link
Member

commented Sep 7, 2016

@AndydeCleyre I don't know, how to make that in an easy way. You need to move all the SourceFiles/ui/text/text_.. on something completely different, that uses only public Qt API.

@john-preston

This comment has been minimized.

Copy link
Member

commented Sep 7, 2016

@AndydeCleyre I guess you need to create a separate QTextDocument object for each displayed text piece and wrap all the methods that work with Text() right now to work with those documents.

@pschichtel

This comment has been minimized.

Copy link

commented Sep 8, 2016

What about upstreaming the changes? If your controls are in fact that much more efficient than the standard Qt ones, I guess a lot of other applications could benefit from them as well.

@john-preston

This comment has been minimized.

Copy link
Member

commented Sep 8, 2016

@pschichtel I'm pretty sure this can't be done :( They're not general controls in any way, just very app-specific pieces of code. Perhaps the approach that was used there could be generalized some how but it is a huge amount of work in its own and after that I still doubt that any of it will be merged to Qt.

@pschichtel

This comment has been minimized.

Copy link

commented Sep 8, 2016

@john-preston Are entire controls implemented in the Qt patches? I thought more about the optimizations you did. Or you could add in public APIs for the private ones you are using and upstream them.
This all sounds like hacking around limitations of a library instead of just removing the limitations from the library, I don't like that.

@WhyNotHugo

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2016

Is is somehow possible to make those "private Qt APIs" public upstream? Has this even been discussed?

@AndydeCleyre

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2016

This is by far the most important bug in this project, and I would contribute to a bug bounty for it.

@Schekolda

This comment has been minimized.

Copy link

commented Mar 31, 2017

So, will you stop using patched QT?

@stek29

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2017

@KireinaHoro

This comment has been minimized.

@AndydeCleyre

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2017

Can someone write here what the deal is with those "systemqt" packages? What works and what doesn't?

@emanuelserpa

This comment has been minimized.

Copy link

commented Sep 15, 2017

It's working great here, I'm not seeing anything not working in https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=telegram-desktop-systemqt
Also, the deal is that it compiles with the official Qt, without patched Qt

@AndydeCleyre

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2017

Wow, it does seem to work perfectly so far. @john-preston can you check this out and point out what we should expect to be broken -- and if nothing (or very little) is, how this might affect the dependency on your custom Qt going forward?

@agura-lex

This comment has been minimized.

Copy link

commented Oct 20, 2017

By the way, using system qt fixes this ancient bug: #1026. It appears to be libappindicator bug, I guess, as systemqt version (https://aur.archlinux.org/packages/telegram-desktop-systemqt) uses native tray icon.

@jhasse

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2017

I don't think that has anything to do with Qt, but if the build uses libappindicator or not.

@emanuelserpa

This comment has been minimized.

Copy link

commented Dec 12, 2017

Any updates?

@portaloffreedom

This comment has been minimized.

Copy link

commented Dec 14, 2017

Not using official Qt actually breaks the program running over wayland

@orschiro

This comment has been minimized.

Copy link

commented Dec 21, 2017

@agur4ik what you were saying is related to this theme inconsistency, correct?

image

@mxa

This comment has been minimized.

Copy link

commented Dec 21, 2017

I suspect that fixing this would also resolve two other bugs which are quite annoying:

  1. make nimf work with Telegram #2603
  2. compose key currently broken in Telegram #2603
@agura-lex

This comment has been minimized.

Copy link

commented Dec 21, 2017

@orschiro, it might be related but I'm not sure as I've not tested that. But since you are using GTK-based environment, theme inconsistency more probably has something to do with different GTK and Qt styles. Here's described how to fix it: https://wiki.archlinux.org/index.php/Uniform_look_for_Qt_and_GTK_applications.
(I hope I got your question right)

@Peque

This comment has been minimized.

Copy link

commented Dec 22, 2017

@john-preston Friendly ping. 😇

@john-preston

This comment has been minimized.

Copy link
Member

commented Dec 22, 2017

@Peque You can read my replies above. I don't know an easy and reliable way of moving this project to the non-custom Qt. Most of the patching is used on Windows and OS X, but some on Linux as well. Right now:

— Text layout uses internal Qt API.. All attempts to work with official Qt up till now still used it, just hoped that the required methods are being present in the dynamic libraries. If Qt some day changes or removes those methods the app would simply fail to launch.

— Open Sans Semibold font that is used widely in the app wouldn't load without patching Qt for that font. I don't know whose fault is that, but I couldn't make it work, Qt saw it just as another "Open Sans" font and all semibold text parts were rendered as normal text (names in chats list etc).

— Detection of the compositing manager was backported from next versions of Qt, so I could know if transparent windows are supported and not try to show them if they aren't.

Fixing bugs only in the upstream Qt is inefficient. If you see bugs / crash reports and you manage to find and fix them in Qt and even if you manage to report and include those fixes in some next version (which requires huge amount of work, I almost never have time for this), your current users still won't be able to use your app.

Also I'll need to deploy current versions anyway, because all current users are using the app and autoupdating to the next versions, if I just remove the linked Qt in some cases the app just won't launch anymore.

@jhasse

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

What about using https://github.com/CrimsonAS/gtkplatform/ for your static Qt version? This way the theming would be picked up from system Gtk at least.

@orschiro

This comment has been minimized.

Copy link

commented Dec 22, 2017

@agur4ik

it might be related but I'm not sure as I've not tested that. But since you are using GTK-based environment, theme inconsistency more probably has something to do with different GTK and Qt styles. Here's described how to fix it: https://wiki.archlinux.org/index.php/Uniform_look_for_Qt_and_GTK_applications.
(I hope I got your question right)

You got it right! However, I tried this already. If I understand correctly, then this would work if Telegram Desktop uses the non-forked version of Qt.

@jhasse

What about using https://github.com/CrimsonAS/gtkplatform/ for your static Qt version? This way the theming would be picked up from system Gtk at least.

Does the user have to use this or the developer?

@Peque

This comment has been minimized.

Copy link

commented Dec 22, 2017

@john-preston Thanks for your reply. Even though I read your comments above, it is good to have an update on this matter after more than one year. Just in case anything had changed. 😉

@emanuelserpa

This comment has been minimized.

Copy link

commented Dec 23, 2017

@john-preston What's your thoughts about https://www.archlinux.org/packages/community/x86_64/telegram-desktop/? They are using official qt now.

@john-preston

This comment has been minimized.

Copy link
Member

commented Dec 25, 2017

@emanuelserpa This looks promising. If I understood correctly they've just copied small amount of code from Qt source that isn't available through Qt public API and compile it inside the binary.

@agura-lex

This comment has been minimized.

Copy link

commented Dec 28, 2017

@emanuelserpa I think, in Arch it is easy to incorporate system Qt Telegram build as it uses the latest and greatest software (@john-preston pointed out that some fuctionality is backported from the newer versions of Qt). Distributing Telegram with native Qt would result in the need to provide different TG builds for the different Qt versions. I never thought about this earlier though. Now I think that the issue should be resolved in downstream rather than upstream: let distro maintainers build TG with native Qt (if they will) and TG devs continue providing static build unified for all distros. However, it might be a good idea to simplify building the app against the different Qt versions by making this thing configurable (if that's easily possible) in order to avoid patching from distro maintainers.

P.S.: sorry for my English.

@stek29

This comment has been minimized.

Copy link
Collaborator

commented Dec 28, 2017

Distributing Telegram with native Qt would result in the need to provide different TG builds for the different Qt versions

It shouldn't. At least when done properly.

@orschiro

This comment has been minimized.

Copy link

commented Dec 28, 2017

let distro maintainers build TG with native Qt (if they will) and TG devs continue providing static build unified for all distros

Let's take Ubuntu as an example. It doesn't provide Telegram Desktop in its official repositories. But as far as I know there is a Snap as well as Flatpak for TD. Does this make a difference to the argument?

@gasinvein

This comment has been minimized.

Copy link

commented Dec 28, 2017

Use of patched Qt is a strong argument against adding Telegram to official repositories. Maybe if Telegram would work with distro's native Qt, there will be no need in Snap or Flatpak for it.

@loimu

This comment has been minimized.

Copy link

commented Dec 28, 2017

Let's take Ubuntu as an example. It doesn't provide Telegram Desktop in its official repositories.

Really? What's that then?

@orschiro

This comment has been minimized.

Copy link

commented Dec 29, 2017

@loimu forget what I just said. Don't even know how I missed that. And I was sure it didn't have the package the last time I was looking for it in the official repos. 😅

@shibotto

This comment has been minimized.

Copy link

commented Feb 22, 2018

ebuilds for Gentoo are available too https://github.com/reagentoo/gentoo-overlay/tree/master/net-im/telegram-desktop
I've been using this for a couple of weeks and not a problem so far.

@gasinvein

This comment has been minimized.

Copy link

commented Mar 20, 2018

So, as for now, Fedora (rpmfusion), openSUSE, Arch, Gentoo, Debian/Ubuntu and even Flathub, all patch Telegram to avoid linking statically with modified Qt. Did I miss someone?

@3v1n0

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2018

We don't do it in the snap so far, since I want to do same as upstream... But of course, that would be probably better.

@balsoft

This comment has been minimized.

Copy link

commented Oct 24, 2018

So, as for now, Fedora (rpmfusion), openSUSE, Arch, Gentoo, Debian/Ubuntu and even Flathub, all patch Telegram to avoid linking statically with modified Qt. Did I miss someone?

You missed NixOS 😄

@mikken

This comment has been minimized.

Copy link

commented Feb 10, 2019

Gentoo only has statically linked official binary package for Telegram Desktop.
The one above is a third-party package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.