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

i18n: use qt6 dir for translations when building on qt6 #2813

Merged
merged 2 commits into from
Aug 21, 2023

Conversation

charliewolf
Copy link
Contributor

hi, i'm working on updating a qownnotes rpm spec to support qt6 and ran into some issues with this hardcoded path.

@pbek
Copy link
Owner

pbek commented Aug 17, 2023

Thank you for taking that challenge on!

Unfortunately, changing that has a lot of implications, since the path is used a lot: https://github.com/search?q=repo%3Apbek%2FQOwnNotes%20%22qt5%2Ftranslations%22&type=code

@charliewolf
Copy link
Contributor Author

@pbek from what I can tell, of the 7 results in the linked search, main.cpp is the only file it would be appropriate to change in this PR. the rest are packaging scripts (arch PKGBUILDs, gentoo port, etc) and those would only need to be changed when the packages built for those distros are updated to use qt6, which would be a very large systemic change that I wouldn't say is in the scope of this PR. i have made the change in main.cpp in a new commit.

src/main.cpp Outdated
@@ -49,8 +49,13 @@ void loadTranslations(QTranslator *translator, const QString &locale) {
loadTranslation(translator[5], appPath + "/languages/QOwnNotes_" + locale);
loadTranslation(translator[6], appPath + "/QOwnNotes_" + locale);
loadTranslation(translator[7], "../src/languages/QOwnNotes_" + locale);
#if QT_VERSION < QT_VERSION_CHECK(6, 0, 0)
Copy link
Owner

Choose a reason for hiding this comment

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

You'll need a >= check here, wouldn't you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, fixed.

Copy link
Owner

Choose a reason for hiding this comment

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

thank you

@pbek
Copy link
Owner

pbek commented Aug 17, 2023

Will need changes too:

"/usr/share/qt5/translations/"

@charliewolf
Copy link
Contributor Author

got it, added a check in loadReleaseTranslations as well.

@pbek
Copy link
Owner

pbek commented Aug 17, 2023

I tested building on NixOS with Qt 6 and for some reason the qt6/translations wasn't even created 🤔 .

@charliewolf
Copy link
Contributor Author

i'm not familiar with nix but this patch worked for me on RHEL 9. did it create the qt5 dir or what did it do?

@pbek
Copy link
Owner

pbek commented Aug 21, 2023

I got it working now. LGTM. Thank you.

@pbek pbek merged commit 329c40b into pbek:main Aug 21, 2023
21 of 26 checks passed
pbek added a commit that referenced this pull request Aug 21, 2023
@pbek
Copy link
Owner

pbek commented Aug 21, 2023

Hm, seems like a lot of Qt6 builds break...
https://github.com/pbek/QOwnNotes/actions/runs/5887832027

@barolo
Copy link

barolo commented Feb 6, 2024

Hm, seems like a lot of Qt6 builds break... https://github.com/pbek/QOwnNotes/actions/runs/5887832027

Hi is support for Qt6 complete now?

@pbek
Copy link
Owner

pbek commented Feb 6, 2024

Hi is support for Qt6 complete now?

Yes, quite a while ago...

@barolo
Copy link

barolo commented Feb 6, 2024

Hi is support for Qt6 complete now?

Yes, quite a while ago...

Oh. Would you have some time to update ebuilds then? They still force qt5.

@pbek
Copy link
Owner

pbek commented Feb 6, 2024

Sure, just send in a PR for the changes in https://github.com/pbek/QOwnNotes/blob/main/build-systems/gentoo/qownnotes.ebuild, I don't really have a Gentoo sitting around. I'm mainly on NixOS. 😉

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.

3 participants