VPN-5472: Merge Mozilla.Shared.qmlcomponents into Mozilla.VPN module #8843

oskirby · 2023-12-19T20:16:19Z

Description

We have received a few reports of users encountering a blank UI screen on some Linux distributions. So far this seems to manifest on Ubuntu 20.04 intermittently. When this occurs, we find a QML error in the log of the form:

Warning: qrc:/ui/screens/ScreenHome.qml:9:1: module "Mozilla.VPN" is not installed (ScreenHome.qml:9)

However, this Mozilla.VPN QML module is created programmatically by the program. I suspect that this error occurs because something in the QML cache isn't able to figure out that this module exists, creating this module with qt6_add_qml_module() might do the trick.

Reference

Github issue: #7912 (VPN-5472)

Checklist

My code follows the style guidelines for this project
I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
I have performed a self review of my own code
I have commented my code PARTICULARLY in hard to understand areas
I have added thorough tests where needed

oskirby · 2023-12-19T21:03:25Z

Bah, humbug. Okay, so this did actually fix the QML cache loading jankiness, but it completely broke the QML_NAMED_ELEMENT() classes from the C++ world. I guess this means I'm on the right track at least.

mcleinman

Looks good. A couple questions so I can better understand this, as I don't know this part of the code super-well.

I don't have a Linux box or VM currently, so I'm not able to test it.

mcleinman · 2024-01-23T20:28:35Z

src/productshandler.cpp

@@ -44,6 +44,11 @@ ProductsHandler::ProductsHandler(QObject* parent) : QAbstractListModel(parent) {
  Q_ASSERT(!s_instance);
  s_instance = this;

+  // If web purchases are supported, then we don't need to do anything.


Why move this in?

I couldn't find a way to conditionally register a singleton type purely using qt_add_qml_module() in a way that would respect the feature flags so I figured the cleanest solution was to register all the QML types in the Mozilla.VPN module (and to do so consistently regardless of feature flagging).

In order to replicate the intended behaviour of the C++ code that this replaces, it was necessary to ensure that the ProductsHandler class doesn't do anything when the feature flag is disabled even if we happen to instantiate it.

mcleinman · 2024-01-23T20:33:30Z

nebula/ui/components/filterproxymodel.cpp

-namespace {
-Logger logger("FilterProxyModel");
-}
+#include <QDebug>


Why the changes in this file?

In order to move this class into the Nebula library, it was important to make sure that the C++ code doesn't depend on code in the mozillavpn target, otherwise this would result in a dependency loop. This mostly just required that we drop the use of the VPN's logging infrastructure and just do it in pure Qt.

mcleinman · 2024-01-23T20:34:28Z

src/addons/addonmessage.h

@@ -22,8 +22,6 @@ constexpr const char* ADDON_MESSAGE_SETTINGS_STATUS_KEY = "state";
 class AddonMessage final : public Addon {
  Q_OBJECT
  Q_DISABLE_COPY_MOVE(AddonMessage)
-  QML_NAMED_ELEMENT(MZAddonMessage)


Why remove this?

Generally I was finding that the QML types registered implicitly by the mozillavpn target were failing to make it into the QML namespace. To work around it, this PR is attempting to move all the QML type registration into the mozillavpn-ui target and we do it in src/ui/types/MZAddonMessage.h with the QML_FOREIGN() macro instead.

mcleinman

Thanks for those explanations. Code looks great to me, but I'm not able to test it - I don't have a Linux box set up.

Naomi, your call on whether you want to merge or wait for additional 👍🏻 on this one.

lesleyjanenorton

And here I was really hoping this was just an issue of adding an import statement or two. Oof. Thanks for taking this on @oskirby!

A few things:

Why move the composer dir into src/ui? Is it possible to move it to nebula/ui instead? NBD if not but so far only QML types in Nebula are prefixed with "MZ". (Another matter for our tech debt pile: we no longer need to separate the QML according to what can be 'shared' or not)
I'm still completely baffled as to why Mozilla.Shared works and Mozilla.VPN randomly fails (although to your point on Slack, maybe Shared fails too and we just don't know it yet).

nebula/ui/components/qmldir

lesleyjanenorton · 2024-01-29T21:54:04Z

tests/qml/CMakeLists.txt

@@ -161,8 +147,6 @@ target_sources(qml_tests PRIVATE
    ${MZ_SOURCE_DIR}/errorhandler.h
    ${MZ_SOURCE_DIR}/feature/feature.cpp
    ${MZ_SOURCE_DIR}/feature/feature.h
-    ${MZ_SOURCE_DIR}/filterproxymodel.cpp


Do we need filterproxymodel.cpp/.h?

We do, it's just included as a part of the nebula library now instead of needing to be manually included in the tests.

lesleyjanenorton · 2024-01-29T22:02:21Z

src/connectionhealth.h

@@ -58,16 +57,7 @@ class ConnectionHealth final : public QObject {

  ConnectionStability stability() const { return m_stability; }

-  void overwriteStabilityForInspector(ConnectionStability stability) {


lesleyjanenorton · 2024-01-29T23:09:31Z

src/commands/commandui.cpp

-      qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNProducts",
-                                   ProductsHandler::instance());
-    }
-


This wasn't introduced by your changes but does it still make sense to register MZAccessibleNotification and MZI18n here instead of qmlengineholder? Seems odd to be handling this in two places.

I agree, it is a bit weird registering things in multiple places like this. However, my ideal solution, if I were up to another refactoring challenge, would be to combine the Mozilla.VPN and Mozilla.Shared modules together. They are vestigial remains of our attempt to modularize the codebase and doing so would require that we register everything in the QML module instead of calling qmlRegisterSingletonInstance()

nebula/ui/components/forms/MZSearchBar.qml

nebula/ui/components/MZGuideList.qml

lesleyjanenorton · 2024-01-30T04:18:21Z

Also want to note: My questions don't need to be answered if you're ready to merge. I'm just baffled and curious/wondering.

oskirby · 2024-01-30T07:47:59Z

* Why move the composer dir into src/ui? Is it possible to move it to nebula/ui instead? NBD if not but so far only QML types in Nebula are prefixed with "MZ". (Another matter for our tech debt pile: we no longer need to separate the QML according to what can be 'shared' or not)

I wanted to; it would have cleaned things up nicely and left all the composer stuff in the nebula library. Unfortunately, there's a dependency between the composer classes and the AddonApi which would result in a dependency loop between nebula and mozillavpn.

oskirby changed the title ~~VPN-5472: Merge Mozilla.Share.qmlcomponents into Mozilla.VPN modules~~ VPN-5472: Merge Mozilla.Share.qmlcomponents into Mozilla.VPN module Dec 19, 2023

oskirby changed the title ~~VPN-5472: Merge Mozilla.Share.qmlcomponents into Mozilla.VPN module~~ VPN-5472: Merge Mozilla.Shared.qmlcomponents into Mozilla.VPN module Dec 19, 2023

oskirby requested review from MattLichtenstein and lesleyjanenorton and removed request for MattLichtenstein and lesleyjanenorton December 19, 2023 20:43

oskirby force-pushed the vpn-5472-ubuntu-focal-blank-screen branch from a451ecc to 63c4433 Compare December 28, 2023 01:11

oskirby marked this pull request as ready for review January 16, 2024 17:51

oskirby marked this pull request as draft January 16, 2024 17:54

oskirby added 17 commits January 17, 2024 23:15

QML: Merge Mozilla.Share.qmlcomponents into Mozilla.VPN modules

5ca6eaa

Move MZFilterProxyModel into nebula

a51b982

A wee touch of lint

e12e64d

Fix filterableModel QML test

ccc2515

Fix typo in QML filter proxy test

2599cf3

Create Mozilla.VPN.ui qml module and move composer classes into it

76b65d7

Fix unit tests

40eb857

Break Addon -> Settings header dependency

7bf0be6

Merge Mozilla.VPN and Mozilla.VPN.ui and use QML_FOREIGN for singletons

91afa0d

Fix dummy client build

0ec5897

Fix QML unit tests

e04c7fb

Fix unit tests

9e0b934

Register MZAddonMessage type through UI module

d1bafc7

Fix some lint

5d89cf2

Add VPNAndroidUtils singleton too

2dec33c

Move MZComposerView back into nebula

4f22df4

Fixup some lint

ebddc2f

oskirby force-pushed the vpn-5472-ubuntu-focal-blank-screen branch from 8829be8 to ebddc2f Compare January 18, 2024 07:16

Make VPNProducts a singleton too

c059d73

oskirby marked this pull request as ready for review January 18, 2024 18:54

oskirby requested review from lesleyjanenorton and mcleinman January 18, 2024 18:55

mcleinman reviewed Jan 23, 2024

View reviewed changes

oskirby requested a review from mcleinman January 29, 2024 16:36

mcleinman approved these changes Jan 29, 2024

View reviewed changes

lesleyjanenorton reviewed Jan 29, 2024

View reviewed changes

lesleyjanenorton approved these changes Jan 30, 2024

View reviewed changes

Put MZCollapsileCard.qml back where we found it

ce44ede

oskirby force-pushed the vpn-5472-ubuntu-focal-blank-screen branch from 1b6583f to ce44ede Compare January 30, 2024 09:38

oskirby merged commit 2d45b94 into main Jan 30, 2024
125 checks passed

oskirby deleted the vpn-5472-ubuntu-focal-blank-screen branch January 30, 2024 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPN-5472: Merge Mozilla.Shared.qmlcomponents into Mozilla.VPN module #8843

VPN-5472: Merge Mozilla.Shared.qmlcomponents into Mozilla.VPN module #8843

oskirby commented Dec 19, 2023 •

edited by jira bot

Loading

oskirby commented Dec 19, 2023

mcleinman left a comment

mcleinman Jan 23, 2024

oskirby Jan 24, 2024

mcleinman Jan 23, 2024

oskirby Jan 23, 2024 •

edited

Loading

mcleinman Jan 23, 2024

oskirby Jan 23, 2024 •

edited

Loading

mcleinman left a comment

lesleyjanenorton left a comment

lesleyjanenorton Jan 29, 2024

oskirby Jan 30, 2024

lesleyjanenorton Jan 29, 2024

lesleyjanenorton Jan 29, 2024

oskirby Jan 30, 2024 •

edited

Loading

lesleyjanenorton commented Jan 30, 2024

oskirby commented Jan 30, 2024

		@@ -58,16 +57,7 @@ class ConnectionHealth final : public QObject {

		ConnectionStability stability() const { return m_stability; }

		void overwriteStabilityForInspector(ConnectionStability stability) {

VPN-5472: Merge Mozilla.Shared.qmlcomponents into Mozilla.VPN module #8843

VPN-5472: Merge Mozilla.Shared.qmlcomponents into Mozilla.VPN module #8843

Conversation

oskirby commented Dec 19, 2023 • edited by jira bot Loading

Description

Reference

Checklist

oskirby commented Dec 19, 2023

mcleinman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskirby Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskirby Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

mcleinman left a comment

Choose a reason for hiding this comment

lesleyjanenorton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskirby Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

lesleyjanenorton commented Jan 30, 2024

oskirby commented Jan 30, 2024

oskirby commented Dec 19, 2023 •

edited by jira bot

Loading

oskirby Jan 23, 2024 •

edited

Loading

oskirby Jan 23, 2024 •

edited

Loading

oskirby Jan 30, 2024 •

edited

Loading