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

Update to qt6 #2782

Merged
merged 16 commits into from
Jul 24, 2023
Merged

Update to qt6 #2782

merged 16 commits into from
Jul 24, 2023

Conversation

andrei-toterman
Copy link
Contributor

@andrei-toterman andrei-toterman commented Oct 17, 2022

Fixes #2627

@@ -45,7 +45,7 @@ class StandardPaths : public Singleton<StandardPaths>
static constexpr auto PicturesLocation = StandardLocation::PicturesLocation;
static constexpr auto TempLocation = StandardLocation::TempLocation;
static constexpr auto HomeLocation = StandardLocation::HomeLocation;
static constexpr auto DataLocation = StandardLocation::DataLocation;
static constexpr auto DataLocation = StandardLocation::AppDataLocation;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand this saves us from changing client code everywhere, but I think we should stick to what we're wrapping. These wrappers exist only for the purpose of testing and I believe we should keep them as thin and faithful to the original as possible. The moment we start mapping things differently or adding functionality, they themselves would need to be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point. Now that I look closer, DataLocation is not used anywhere in our code and the QStandardPaths::StandardLocation::DataLocation has been removed in favor of QStandardPaths::StandardLocation::AppDataLocation and we already have a AppDataLocation constant that wraps that value. So I guess the DataLocation can be removed entirely, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh ok, yep I think it should be removed if it is not available in the wrappee anymore.

@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Merging #2782 (4b92337) into main (82495d0) will decrease coverage by 0.06%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main    #2782      +/-   ##
==========================================
- Coverage   88.55%   88.49%   -0.06%     
==========================================
  Files         239      239              
  Lines       12158    12111      -47     
==========================================
- Hits        10766    10718      -48     
- Misses       1392     1393       +1     
Impacted Files Coverage Δ
src/daemon/daemon.h 100.00% <ø> (ø)
src/network/url_downloader.cpp 100.00% <ø> (ø)
...rc/platform/backends/qemu/qemu_virtual_machine.cpp 68.52% <0.00%> (ø)
src/settings/wrapped_qsettings.h 18.51% <ø> (+1.85%) ⬆️
...tform/backends/libvirt/libvirt_virtual_machine.cpp 64.00% <50.00%> (ø)
src/daemon/daemon.cpp 73.56% <84.21%> (-0.07%) ⬇️
src/client/cli/argparser.cpp 96.34% <100.00%> (-0.61%) ⬇️
src/client/cli/cmd/launch.cpp 87.58% <100.00%> (ø)
src/client/cli/cmd/mount.cpp 92.30% <100.00%> (ø)
src/client/cli/cmd/set.cpp 95.55% <100.00%> (ø)
... and 8 more

... and 10 files with indirect coverage changes

@andrei-toterman andrei-toterman force-pushed the update-snap-to-core22 branch 3 times, most recently from 21a93a0 to f5b5882 Compare October 31, 2022 14:13
@andrei-toterman andrei-toterman force-pushed the update-snap-to-core22 branch 2 times, most recently from d619899 to c67e2f0 Compare October 31, 2022 18:28
@andrei-toterman andrei-toterman force-pushed the update-to-qt6 branch 3 times, most recently from b7a5696 to d66a3f4 Compare January 11, 2023 12:46
@andrei-toterman andrei-toterman marked this pull request as draft January 17, 2023 14:07
@andrei-toterman andrei-toterman marked this pull request as ready for review January 17, 2023 14:07
@andrei-toterman andrei-toterman force-pushed the update-to-qt6 branch 3 times, most recently from 41067f2 to 0def63d Compare January 17, 2023 19:15
@bors bors bot changed the base branch from update-snap-to-core22 to main January 25, 2023 15:57
Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

Hey @andrei-toterman!

Thanks for this! Unfortunately, the tray icon GUI does not work for me:

$ multipass.gui 
Warning: Ignoring WAYLAND_DISPLAY on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
qt.qpa.plugin: Could not find the Qt platform plugin "xcb" in ""
qt.qpa.plugin: Could not find the Qt platform plugin "wayland" in ""
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Aborted (core dumped)

@townsend2010 townsend2010 marked this pull request as draft January 30, 2023 19:17
@townsend2010 townsend2010 marked this pull request as ready for review January 30, 2023 19:17
@townsend2010 townsend2010 linked an issue Feb 8, 2023 that may be closed by this pull request
it was needed to set the codec to UTF-8 but that became the default in
Qt6
in Qt6 overloading QProcess::setupChildProcess has been replaced with
passing a function to setChildProcessModifier
in Qt6, that attribute has been removed and redirects are followed by
default
in Qt6, QFuture operator== has been removed, so we can't directly
compare if two QFutures are the same. now, when a QFutureWatcher is
created, we create a UUID to identify it in a map
@townsend2010 townsend2010 marked this pull request as draft July 24, 2023 13:17
@townsend2010 townsend2010 marked this pull request as ready for review July 24, 2023 13:17
Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

This fails building on the private side due to the private side change in the workflow.

I'm gonna bors this to get the commit message, but it will fail, so I'll just merge manually.

@townsend2010
Copy link
Contributor

bors merge

bors bot added a commit that referenced this pull request Jul 24, 2023
2782: Update to qt6 r=townsend2010 a=andrei-toterman

Fixes #2627

Co-authored-by: Andrei Toterman <[email protected]>
@townsend2010 townsend2010 merged commit 77cb016 into main Jul 24, 2023
20 of 23 checks passed
@bors bors bot deleted the update-to-qt6 branch July 24, 2023 16:14
@bors
Copy link
Contributor

bors bot commented Jul 24, 2023

Timed out.

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.

Update to Qt6
3 participants