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

core: use qt libraries and follow qml module conventions #8

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

outfoxxed
Copy link
Member

@outfoxxed outfoxxed commented Oct 14, 2024

Uses a qml module instead of a context property.
Replaces usages of std and hyprutils with qtbase where applicable. Removes dependencies: hyprutils, wl-copy, whoami, uname.
Rolls c++ version back to 20, as 23 isn't currently required for anything.
Fixes the image provider.

I figure you're going to ask why for a couple of these so:

  • Code was moved into src/ as the qt moc does not work correctly if CMakeLists.txt isn't in the same folder.
  • We're using a qml module instead of a context property because it's conventionally correct and enables optimizations (though that part doesn't matter much for this application)
  • We're using qt libraries over std+hyprutils to avoid repeatedly translating strings between utf8 and utf16, and the fact that qt already provides everything we were using from them.
  • The nix changes here stand on their own and are tested. nix: fix package and add devshell #5 is not required.

This should functionally be a refactor. Nothing user facing has changed.

Uses a qml module instead of a context property.
Replaces usages of std and hyprutils with qtbase where applicable.
Removes dependencies: hyprutils, wl-copy, whoami, uname.
Rolls c++ version back to 20, as 23 isn't currently required for anything.
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

Code was moved into src/ as the qt moc does not work correctly if CMakeLists.txt isn't in the same folder.

👍

We're using a qml module instead of a context property because it's conventionally correct and enables optimizations (though that part doesn't matter much for this application)

👍

We're using qt libraries over std+hyprutils to avoid repeatedly translating strings between utf8 and utf16, and the fact that qt already provides everything we were using from them.

I am very much not a fan of this. We are not KDE, where qt is our primary dependency in all apps.

The point of hyprutils and std usage is that this is what we use across the ecosystem.

Using qt equivalents introduces many problems:

  • many qt equivalents are very unsafe. The more we use them, the more problems. Qt passes raw pointers around often.
  • qt equivalents require the developer to learn a whole new suite of replacement. It's not a 1:1 with std.
    • expanding on that: I have trouble reading your code, because I am not familiar with qt C++ stuff. You are, I am not, and the average hyprland/hypr* contributor shouldn't be required to.

Some code changes are cool and all, for example using utsname instead of executing uname -a, yeah. However, most of the stuff (QFile, QTextStream) are not a thing I want to use.

Yes, std::string -> QString implies translation, but that can be solved by minimizing the passing of those strings, instead of rewriting everything and forcing the learning a new library. Furthermore, on modern PCs, this translation will be an absolutely insignificant performance hit.

@outfoxxed
Copy link
Member Author

outfoxxed commented Oct 14, 2024

many qt equivalents are very unsafe. The more we use them, the more problems. Qt passes raw pointers around often.

This one is not an issue the way they are used in this pr. In general the utility parts of qt don't suffer much from hard to use pointers.

expanding on that: I have trouble reading your code, because I am not familiar with qt C++ stuff. You are, I am not, and the average hyprland/hypr* contributor shouldn't be required to.

I'm going to assume the main issues are QString("").arg(), first, and left, simplified along with QTextStream().operator<< are your main gripes here moreso than QFile.

Personally I've tried mix-n-matching qt and other ecosystems and my opinion is that it usually works best when you lean into it, but that doesn't mean we have to use less common/obvious qt functions. e.g. I don't see an issue with the way QFile or QTextStream is used for reading (certainly easier to read than whatever you were doing with io streams), but maybe I'm missing something that you don't consider obvious.

Would you be willing to compromise here? Something along the lines of:

  • use std::format over .arg or <<
  • use cvarlist when a single .split won't cut it
  • use sliced over first, left, etc
  • anything else?

@vaxerski
Copy link
Member

I would prefer to not use Q* whenever possible (and reasonable, aka doesnt come with a idk 2s lagspike)

@outfoxxed
Copy link
Member Author

I think that's an overly extreme stance, especially when it offers a reasonable alternative. I've tried it before when I was first learning qt and it leads to a lot of unnecessary friction.

I think the familliarity argument falls apart with most of it as well, e.g. I don't think theres much benefit to assuming someone is familliar with std ifstreams over qfile, especially with names like readLine and atEnd.

@outfoxxed
Copy link
Member Author

Rolled back most std replacement changes as we discussed

src/SystemInfo.cpp Outdated Show resolved Hide resolved
src/SystemInfo.cpp Outdated Show resolved Hide resolved
@outfoxxed outfoxxed force-pushed the qmlmodule branch 2 times, most recently from 11b24e5 to 5d6473b Compare October 15, 2024 10:30
Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

king of the jungle

@vaxerski vaxerski merged commit 00beba9 into hyprwm:main Oct 15, 2024
1 check passed
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.

2 participants