-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
lomiri.lomiri-ui-toolkit: init at 1.3.5011 #241717
lomiri.lomiri-ui-toolkit: init at 1.3.5011 #241717
Conversation
faa5f71
to
83f736a
Compare
7de2caf
to
31004ed
Compare
I've cleaned stuff up a tiny bit by playing around with the tests and commented some stuff. The huge amount of disabled tests likely needs to stay - I've checked them all in a less-pure I think this is more or less ready. I will "backport" the changes I've made since the first commit to my big Lomiri branch to test that it doesn't break stuff too much (the localisation one might need some changes, but it'll work abit better than before), then squash away all the commits and mark it ready. |
27cc9ca
to
06aa367
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/2498 |
4189ff6
to
8b30b1e
Compare
8b30b1e
to
b4674ca
Compare
Result of 2 packages built:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh wow that's a lot, first of all thank you for your amazing work!
I tired reviewing it but I'm not at all familiar with lomiri and the ecosystem around it, and hope the comments help. I would encourage you asking some one else to also review this, maybe some one that is also familiar with the prior art because I don't really feel comfortable merging this because of lack of knowledge and sadly won't have time to get deeply involved.
# Using /run/current-system/sw/share/locale instead of /usr/share/locale isn't a great | ||
# solution, but at least it should get us working localisations | ||
substituteInPlace src/LomiriToolkit/i18n.cpp \ | ||
--replace "/usr" "/run/current-system/sw" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you know how other desktop environments on nixos handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closest functional & situational equivalent in a similar DE seems to be ki18n
for KDE, it uses a less-hardcoded way of finding a matching locale prefix? Assuming it works for KDE (haven't tried), I can try to port its method to LUITK and see if that improves things.
Otherwise, our Cinnamon DE packaging also patches such hardcoded locale dirs to /run/current-system/sw/share/locale
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0001-LomiriToolkit-i18n-Locate-locale-via-QStandardPaths-.patch.txt is a quick attempt at porting the ki18n way of doing this to LUITK. I think it should work, but it'll need a setup hook like in ECM to bring the paths of QML dependencies into XDG_DATA_DIRS
(worst case, /run/current-system/sw/share
is already in there so it would be on-par with current submission), and I'm not sure this is acceptable for upstream as-is.
There might also be an existing-but-seemingly-underused way of making sure the domains are properly bound which might be lower-maintenance, but I haven't played around with that yet.
This is definitely doable, but I'll lack the free time to properly finish this up with proper testing of everything, setup hooks & upstreaming now that new university semester has started. I hope the current submission is flawed but good enough for now.
Edit: I've written a note about this on the Lomiri tracking issue, so we don't forget this issue down the road.
b4674ca
to
ad6fe19
Compare
Comments adjusted based on feedback. New version has been released today, including the bump. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1197 |
be8dbde
to
6706fd5
Compare
c46caae
to
8c08179
Compare
8c08179
to
c7b1373
Compare
@wineee Could I bother you for a re-review / ACK of the changes? I submitted some of the distro-agnostic fixes from here to upstream which we now |
Result of 3 packages built:
|
qtfeedback | ||
qtgraphicaleffects | ||
qtsvg | ||
qtwayland |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to confirm these need propagate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- feedback & graphicaleffects should show up when you grep the installed qml files for import statements. they're propagated so individual applications don't need to figure out via trial-and-error which modules they depend upon also happen to require feedback / graphicaleffects
- svg... i don't remember, been too long 🙁. i think for svg icon format support? Can try to check.
- wayland so all applications using the toolkit work on wayland, outside of Lomiri
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrapQtAppsHook will propagate qtwayland in linux
nixpkgs/pkgs/development/libraries/qt-5/5.15/default.nix
Lines 355 to 357 in b206336
name = "wrap-qt5-apps-hook"; | |
propagatedBuildInputs = [ qtbase.dev makeBinaryWrapper ] | |
++ lib.optional stdenv.isLinux qtwayland.dev; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may have fixed usage of lomiri.propagatedBuildInputs
for pre-check setup in some reverse dependencies actually… ehh. Building lomiri-terminal-app
without re-propagated qtwayland
produces wrapping that looks identical so you're right. I'm sure if whatever that was fixing comes up again, I'll notice it again. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
3bda209
to
565b3e3
Compare
Rebased to resolve merge conflict. |
wrapQtAppsHook will already take care of that in applications.
565b3e3
to
55af414
Compare
Result of 3 packages built:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description of changes
Working towards #99090.
Lomiri UI Toolkit, QML components to ease the creation of beautiful applications in QML for Lomiri. Required by most Lomiri core applications.
It more or less passes now, but we need to disable a large amount of tests because they expect a working Qt OpenGL context, which our sandbox doesn't provide.
gettext
-based localisation is broken. Partly.It has some solution now, by looking in
/run/current-system/sw/share/locale
. Not great, but should make those edge cases work without intensive patching.Partly solved. There are variables to control the paths, but some of them get overwritten by the Qt modules they use.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)