D14513: Add QML Extensions API
David Rosca
noreply at phabricator.kde.org
Fri Aug 10 16:54:16 BST 2018
drosca added inline comments.
INLINE COMMENTS
> qmlbookmarks.cpp:233
> + int itemsLength = items.length();
> + for (int i = 0; i < itemsLength; i ++) {
> + BookmarkItem *item = items.at(i);
Why did you change the for loop? Unless there is a reason, don't use for(int i = 0, ....)
> qmlbrowseraction.h:164
> + QQmlComponent* m_popup = nullptr;
> + Locations m_locations;
> + QmlBrowserActionButton *m_button = nullptr;
Missing initialization
> qmlcookie.h:23
> +#include <QNetworkCookie>
> +#include <QDebug>
> +
This should not be needed.
> drosca wrote in qmlextensionscheme.cpp:30
> Where is this deleted?
The question still stands? If it doesn't get to the QML engine, it will leak.
Example where it may happen (I did not test it myself, so plase check) is if you instantiate `Falkon.ExtensionScheme { }` item in QML but don't create the `requestStarted` handler (eg. no `onRequestStarted` code).
> drosca wrote in qmlfileutils.cpp:27
> You already know this from QmlEngine.
?
> qmlaction.h:33
> public:
> - explicit PluginsManager(QWidget* parent = 0);
> - ~PluginsManager();
> -
> - void load();
> - void save();
> -
> -private Q_SLOTS:
> - void settingsClicked();
> - void currentChanged(QListWidgetItem* item);
> - void itemChanged(QListWidgetItem* item);
> -
> - void refresh();
> + explicit QmlAction(QAction *action, QmlEngine *engine, QObject *parent = nullptr);
> + void setProperties(const QVariantMap &map);
Why? You can get the QmlEngine pointer with `qmlEngine(QObject*)` function that is called on any QObject that was created by the engine.
> qmlsettings.cpp:31
> +{
> + m_settings->deleteLater();
> +}
Why not just create QSettings with correct parent?
> qmlsidebar.cpp:136
> + action->setCheckable(m_checkable);
> + if (QIcon::hasThemeIcon(m_iconUrl)) {
> + action->setIcon(QIcon::fromTheme(m_iconUrl));
This seems like the same code for handling icon loading is pasted on multiple places, should be extracted to function.
> qmlsidebar.h:120
> + QString m_shortcut;
> + bool m_checkable;
> + QQmlComponent *m_item = nullptr;
init
> qmltabs.cpp:323
> +
> + for (BrowserWindow *window : staticData->windowIdHash().keys()) {
> + if (staticData->windowIdHash().value(window) == windowId) {
Again, use stl iterators. This creates temporary list only to iterate over it, that's not needed at all.
> qmluserscripts.h:87
> +
> + QList<QObject *> toQObjectList(const QList<QWebEngineScript> &list) const;
> +};
`QList<QObject*>`
> qmlplugin.h:29
> private:
> - void sortItems();
> -
> - Ui::PluginsList* ui;
> - bool m_loaded;
> + static bool m_qmlSupportLoaded;
> };
Just make it static variable in `loadPlugin` function as it is not used anywhere else.
> qmlplugininterface.cpp:42
> +QmlPluginInterface::QmlPluginInterface()
> + : m_qmlReusableTab(new QmlTab())
> +{
Why this at all? It should just use staticData->getTab!
> qmlstaticdata.cpp:36
> + connect(mApp->plugins(), &PluginProxy::mainWindowCreated, this, [this](BrowserWindow *window) {
> + m_windowIdHash.insert(window, m_newWindowId ++);
> + });
There should be no whitespace before/after increment, so it should be `foo++`.
> qmlstaticdata.h:62
> +inline uint qHash(QNetworkCookie cookie) {
> + qDebug() << QString::fromUtf8(cookie.toRawForm());
> + return qHash(QString::fromUtf8(cookie.toRawForm()));
remove
> qmlstaticdata.h:66
> +
> +Q_GLOBAL_STATIC(QmlStaticData, staticData)
It should be something like `static QmlStaticData* instance()` member function and not adding `staticData` to global namespace.
REPOSITORY
R875 Falkon
REVISION DETAIL
https://phabricator.kde.org/D14513
To: anmolgautam, drosca
Cc: falkon, herrold, anmolgautam, SGOrava, iodelay, spoorun, ptabis, navarromorales, cochise, clivej, ach
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/falkon/attachments/20180810/f5a6442c/attachment.html>
More information about the Falkon
mailing list