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.

Sign up for GitHub

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

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

Merged
merged 19 commits into from
Jan 30, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
5ca6eaa
QML: Merge Mozilla.Share.qmlcomponents into Mozilla.VPN modules
oskirby Dec 19, 2023
a51b982
Move MZFilterProxyModel into nebula
oskirby Dec 19, 2023
e12e64d
A wee touch of lint
oskirby Dec 19, 2023
ccc2515
Fix filterableModel QML test
oskirby Dec 20, 2023
2599cf3
Fix typo in QML filter proxy test
oskirby Dec 27, 2023
76b65d7
Create Mozilla.VPN.ui qml module and move composer classes into it
oskirby Dec 27, 2023
40eb857
Fix unit tests
oskirby Dec 27, 2023
7bf0be6
Break Addon -> Settings header dependency
oskirby Dec 28, 2023
91afa0d
Merge Mozilla.VPN and Mozilla.VPN.ui and use QML_FOREIGN for singletons
oskirby Jan 2, 2024
0ec5897
Fix dummy client build
oskirby Jan 3, 2024
e04c7fb
Fix QML unit tests
oskirby Jan 3, 2024
9e0b934
Fix unit tests
oskirby Jan 3, 2024
d1bafc7
Register MZAddonMessage type through UI module
oskirby Jan 3, 2024
5d89cf2
Fix some lint
oskirby Jan 3, 2024
2dec33c
Add VPNAndroidUtils singleton too
oskirby Jan 3, 2024
4f22df4
Move MZComposerView back into nebula
oskirby Jan 4, 2024
ebddc2f
Fixup some lint
oskirby Jan 4, 2024
c059d73
Make VPNProducts a singleton too
oskirby Jan 18, 2024
ce44ede
Put MZCollapsileCard.qml back where we found it
oskirby Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion nebula/ui/components/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ qt_add_qml_module(components
MZChevron.qml
MZClickableRow.qml
MZCollapsibleCard.qml
MZComposerView.qml
MZDropShadowWithStates.qml
MZExternalLinkListItem.qml
MZFlickable.qml
Expand Down Expand Up @@ -82,7 +83,6 @@ qt_add_qml_module(components
MZSwipeDelegate.qml
MZSwipeAction.qml
MZBadge.qml
MZComposerView.qml
MZPasteButton.qml
MZInformationCard.qml
MZStepProgressBar.qml
Expand All @@ -92,6 +92,9 @@ qt_add_qml_module(components
navigationBar/MZBottomNavigationBarButton.qml
navigator/MZNavigatorLoader.qml
navigator/navigatorLoaderInternal.qml
SOURCES
filterproxymodel.cpp
filterproxymodel.h
)

add_subdirectory(forms)
Expand Down
2 changes: 1 addition & 1 deletion nebula/ui/components/MZComposerView.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import QtQuick 2.15
import QtQuick.Layouts 1.15

import Mozilla.Shared 1.0
import Mozilla.Shared.qmlcomponents 1.0
import Mozilla.VPN 1.0
import components 0.1

ColumnLayout {
Expand Down
2 changes: 1 addition & 1 deletion nebula/ui/components/MZGuideList.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import QtQuick 2.15
import QtQuick.Layouts 1.15

import Mozilla.Shared 1.0
import Mozilla.Shared.qmlcomponents 1.0
import Mozilla.VPN 1.0
lesleyjanenorton marked this conversation as resolved.
Show resolved Hide resolved
import components 0.1

GridLayout {
Expand Down
37 changes: 12 additions & 25 deletions src/filterproxymodel.cpp → nebula/ui/components/filterproxymodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,9 @@

#include "filterproxymodel.h"

#include "logger.h"
#include "qmlengineholder.h"

namespace {
Logger logger("FilterProxyModel");
}
#include <QDebug>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the changes in this file?

Copy link
Collaborator Author

@oskirby oskirby Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

#include <QQmlEngine>
#include <QtQml>

FilterProxyModel::FilterProxyModel(QObject* parent)
: QSortFilterProxyModel(parent) {}
Expand All @@ -18,8 +15,7 @@ QJSValue FilterProxyModel::filterCallback() const { return m_filterCallback; }

void FilterProxyModel::setFilterCallback(QJSValue filterCallback) {
if (!filterCallback.isCallable()) {
logger.error()
<< "FilterProxyModel.filterCallback must be a JS callable value";
qWarning() << "FilterProxyModel.filterCallback must be a JS callable value";
return;
}

Expand All @@ -33,8 +29,7 @@ QJSValue FilterProxyModel::sortCallback() const { return m_sortCallback; }

void FilterProxyModel::setSortCallback(QJSValue sortCallback) {
if (!sortCallback.isCallable()) {
logger.error()
<< "FilterProxyModel.sortCallback must be a JS callable value";
qWarning() << "FilterProxyModel.sortCallback must be a JS callable value";
return;
}

Expand Down Expand Up @@ -81,7 +76,7 @@ bool FilterProxyModel::filterAcceptsRow(
}

if (m_filterCallback.isNull() || m_filterCallback.isUndefined()) {
logger.debug() << "No filter callback set!";
qDebug() << "No filter callback set!";
return true;
}

Expand All @@ -92,19 +87,14 @@ bool FilterProxyModel::filterAcceptsRow(
return false;
}

if (!QmlEngineHolder::exists()) {
logger.error() << "Something bad is happening. Are we shutting down?";
return false;
}

QJSValue value = dataToJSValue(sourceModel(), index);

QJSValueList arguments;
arguments.append(value);

QJSValue retValue = m_filterCallback.call(arguments);
if (retValue.isError()) {
logger.debug() << "Execution throws an error:" << retValue.toString();
qDebug() << "Execution throws an error:" << retValue.toString();
return false;
}

Expand All @@ -123,11 +113,6 @@ bool FilterProxyModel::lessThan(const QModelIndex& left,

Q_ASSERT(m_sortCallback.isCallable());

if (!QmlEngineHolder::exists()) {
logger.error() << "Something bad is happening. Are we shutting down?";
return false;
}

QJSValue valueA = dataToJSValue(sourceModel(), left);
QJSValue valueB = dataToJSValue(sourceModel(), right);

Expand All @@ -137,7 +122,7 @@ bool FilterProxyModel::lessThan(const QModelIndex& left,

QJSValue retValue = m_sortCallback.call(arguments);
if (retValue.isError()) {
logger.debug() << "Execution throws an error:" << retValue.toString();
qDebug() << "Execution throws an error:" << retValue.toString();
return false;
}

Expand All @@ -157,8 +142,10 @@ void FilterProxyModel::componentComplete() {

QJSValue FilterProxyModel::dataToJSValue(const QAbstractItemModel* model,
const QModelIndex& index) const {
QJSEngine* engine = QmlEngineHolder::instance()->engine();
Q_ASSERT(engine);
QQmlEngine* engine = qmlEngine(this);
if (engine == nullptr) {
return QJSValue(QJSValue::UndefinedValue);
}

QJSValue value = engine->newObject();
Q_ASSERT(value.isObject());
Expand Down
0 src/filterproxymodel.h → nebula/ui/components/filterproxymodel.h
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion nebula/ui/components/forms/MZSearchBar.qml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import QtQuick 2.0
import QtQuick.Layouts 1.14

import Mozilla.Shared 1.0
import Mozilla.Shared.qmlcomponents 1.0
import Mozilla.VPN 1.0
lesleyjanenorton marked this conversation as resolved.
Show resolved Hide resolved
import components 0.1
import components.forms 0.1

Expand Down
6 changes: 2 additions & 4 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,7 @@ if(NOT CMAKE_CROSSCOMPILING)
target_compile_definitions(mozillavpn PUBLIC MVPN_WEBEXTENSION)
endif()

qt6_add_qml_module(mozillavpn
URI Mozilla.Shared.qmlcomponents
VERSION 1.0
)
add_subdirectory(ui)
target_link_libraries(mozillavpn PRIVATE mozillavpn-ui mozillavpn-uiplugin)

qt_finalize_target(mozillavpn)
3 changes: 1 addition & 2 deletions src/addons/addon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,13 @@
#include <QObject>
#include <QTranslator>

#include "settings/settinggroup.h"
#include "settingsholder.h"
#include "state/addonstate.h"

class AddonConditionWatcher;
class QJsonObject;

class AddonApi;
class SettingGroup;

// A settings group that will contain all settings related to individual addons.
constexpr const char* ADDON_SETTINGS_GROUP = "addon";
Expand Down
2 changes: 1 addition & 1 deletion src/addons/addonguide.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
#ifndef ADDONGUIDE_H
#define ADDONGUIDE_H

#include "../composer/composer.h"
#include "addon.h"
#include "addonproperty.h"
#include "composer.h"

class QJsonObject;

Expand Down
6 changes: 2 additions & 4 deletions src/addons/addonmessage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@
#ifndef ADDONMESSAGE_H
#define ADDONMESSAGE_H

#include "../composer/composer.h"
#include "addon.h"
#include "addonproperty.h"
#include "settingsholder.h"
#include "composer.h"

class QJsonObject;
class SettingGroup;

#ifdef UNIT_TEST
class TestAddon;
Expand All @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Collaborator Author

@oskirby oskirby Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

QML_UNCREATABLE("")

public:
ADDON_PROPERTY(title, m_title, getTitle, setTitle, retranslationCompleted)
Expand Down
2 changes: 2 additions & 0 deletions src/cmake/sentry.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ if( ${_SUPPORTED} GREATER -1 )
target_compile_definitions(shared-sources INTERFACE SENTRY_ENABLED)
# Let's the app know we need to provide the upload client
target_compile_definitions(shared-sources INTERFACE SENTRY_NONE_TRANSPORT)
# We need QML private headers for QML backtracing.
target_link_libraries(shared-sources INTERFACE Qt6::QmlPrivate)

# Sentry support is given
target_sources(shared-sources INTERFACE
Expand Down
16 changes: 0 additions & 16 deletions src/cmake/shared-sources.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,20 +83,6 @@ target_sources(shared-sources INTERFACE
${CMAKE_SOURCE_DIR}/src/authenticationlistener.h
${CMAKE_SOURCE_DIR}/src/collator.cpp
${CMAKE_SOURCE_DIR}/src/collator.h
${CMAKE_SOURCE_DIR}/src/composer/composer.cpp
${CMAKE_SOURCE_DIR}/src/composer/composer.h
${CMAKE_SOURCE_DIR}/src/composer/composerblock.cpp
${CMAKE_SOURCE_DIR}/src/composer/composerblock.h
${CMAKE_SOURCE_DIR}/src/composer/composerblockbutton.cpp
${CMAKE_SOURCE_DIR}/src/composer/composerblockbutton.h
${CMAKE_SOURCE_DIR}/src/composer/composerblockorderedlist.cpp
${CMAKE_SOURCE_DIR}/src/composer/composerblockorderedlist.h
${CMAKE_SOURCE_DIR}/src/composer/composerblocktext.cpp
${CMAKE_SOURCE_DIR}/src/composer/composerblocktext.h
${CMAKE_SOURCE_DIR}/src/composer/composerblocktitle.cpp
${CMAKE_SOURCE_DIR}/src/composer/composerblocktitle.h
${CMAKE_SOURCE_DIR}/src/composer/composerblockunorderedlist.cpp
${CMAKE_SOURCE_DIR}/src/composer/composerblockunorderedlist.h
${CMAKE_SOURCE_DIR}/src/cryptosettings.cpp
${CMAKE_SOURCE_DIR}/src/cryptosettings.h
${CMAKE_SOURCE_DIR}/src/curve25519.cpp
Expand All @@ -107,8 +93,6 @@ target_sources(shared-sources INTERFACE
${CMAKE_SOURCE_DIR}/src/errorhandler.h
${CMAKE_SOURCE_DIR}/src/feature/feature.cpp
${CMAKE_SOURCE_DIR}/src/feature/feature.h
${CMAKE_SOURCE_DIR}/src/filterproxymodel.cpp
${CMAKE_SOURCE_DIR}/src/filterproxymodel.h
${CMAKE_SOURCE_DIR}/src/fontloader.cpp
${CMAKE_SOURCE_DIR}/src/fontloader.h
${CMAKE_SOURCE_DIR}/src/frontend/navigator.cpp
Expand Down
55 changes: 0 additions & 55 deletions src/commands/commandui.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,61 +309,6 @@ int CommandUI::run(QStringList& tokens) {
engine->addImageProvider(QString("app"), provider);
}

qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPN",
MozillaVPN::instance());
qmlRegisterSingletonInstance(
"Mozilla.VPN", 1, 0, "VPNCaptivePortal",
MozillaVPN::instance()->captivePortalDetection());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNController",
MozillaVPN::instance()->controller());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNUser",
MozillaVPN::instance()->user());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNDeviceModel",
MozillaVPN::instance()->deviceModel());

qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0,
"VPNRecentConnectionsModel",
RecentConnections::instance());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0,
"VPNRecommendedLocationModel",
RecommendedLocationModel::instance());
qmlRegisterSingletonInstance(
"Mozilla.VPN", 1, 0, "VPNSupportCategoryModel",
MozillaVPN::instance()->supportCategoryModel());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNServerCountryModel",
MozillaVPN::instance()->serverCountryModel());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNSubscriptionData",
MozillaVPN::instance()->subscriptionData());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNProfileFlow",
MozillaVPN::instance()->profileFlow());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNCurrentServer",
MozillaVPN::instance()->serverData());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNServerLatency",
MozillaVPN::instance()->serverLatency());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNConnectionHealth",
MozillaVPN::instance()->connectionHealth());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNReleaseMonitor",
MozillaVPN::instance()->releaseMonitor());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNAppPermissions",
AppPermission::instance());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNConnectionBenchmark",
MozillaVPN::instance()->connectionBenchmark());
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNIPAddressLookup",
MozillaVPN::instance()->ipAddressLookup());

#ifdef MZ_ANDROID
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNAndroidUtils",
AndroidUtils::instance());
#endif

qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNPurchase",
PurchaseHandler::instance());

if (!Feature::get(Feature::Feature_webPurchase)->isSupported()) {
qmlRegisterSingletonInstance("Mozilla.VPN", 1, 0, "VPNProducts",
ProductsHandler::instance());
}

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@oskirby oskirby Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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()

qmlRegisterSingletonInstance("Mozilla.Shared", 1, 0,
"MZAccessibleNotification",
AccessibleNotification::instance());
Expand Down
13 changes: 13 additions & 0 deletions src/connectionhealth.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <QDateTime>
#include <QRandomGenerator>

#include "constants.h"
#include "controller.h"
#include "glean/generated/metrics.h"
#include "leakdetector.h"
Expand Down Expand Up @@ -262,3 +263,15 @@ void ConnectionHealth::applicationStateChanged(Qt::ApplicationState state) {
}
#endif
}

void ConnectionHealth::overwriteStabilityForInspector(
ConnectionStability stability) {
if (Constants::inProduction()) {
qFatal(
"Connection health stability mode can only be overwritten in Dev "
"mode!");
}
m_stabilityOverwritten = true;
m_stability = stability;
emit stabilityChanged();
}
12 changes: 1 addition & 11 deletions src/connectionhealth.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
#ifndef CONNECTIONHEALTH_H
#define CONNECTIONHEALTH_H

#include "constants.h"
#include "dnspingsender.h"
#include "pinghelper.h"

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

ConnectionStability stability() const { return m_stability; }

void overwriteStabilityForInspector(ConnectionStability stability) {
Copy link
Member

Choose a reason for hiding this comment

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

🧹

if (Constants::inProduction()) {
qFatal(
"Connection health stability mode can only be overwritten in Dev "
"mode!");
}
m_stabilityOverwritten = true;
m_stability = stability;
emit stabilityChanged();
}
void overwriteStabilityForInspector(ConnectionStability stability);

uint latency() const { return m_pingHelper.latency(); }
double loss() const { return m_pingHelper.loss(); }
Expand Down
4 changes: 1 addition & 3 deletions src/mozillavpn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,7 @@ MozillaVPN::MozillaVPN() : App(nullptr), m_private(new MozillaVPNPrivate()) {
&m_private->m_connectionHealth,
&ConnectionHealth::connectionStateChanged);

if (!Feature::get(Feature::Feature_webPurchase)->isSupported()) {
ProductsHandler::createInstance();
}
ProductsHandler::createInstance();
PurchaseHandler* purchaseHandler = PurchaseHandler::createInstance();
connect(purchaseHandler, &PurchaseHandler::subscriptionStarted, this,
&MozillaVPN::subscriptionStarted);
Expand Down
5 changes: 5 additions & 0 deletions src/productshandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why move this in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

if (Feature::get(Feature::Feature_webPurchase)->isSupported()) {
return;
}

// On iAP make sure the Products are loaded in time.
// If we Move into any State adjecent to iAP - load the
// products if we don't have that already.
Expand Down
Loading
Loading