Add QML Extensions API
Details
Diff Detail
- Repository
- R875 Falkon
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
src/lib/plugins/qml/api/i18n/qmli18n.cpp | ||
---|---|---|
34 | const It should also check if the env var was actually set, before trying to reset it. |
src/lib/plugins/qml/api/i18n/qmli18n.cpp | ||
---|---|---|
34 | And if its set before then it shouldn't set it to QLocale::system? |
src/lib/plugins/qml/api/i18n/qmli18n.cpp | ||
---|---|---|
34 | It should always set it, this is meant to force language that we want. |
src/lib/plugins/qml/api/i18n/qmli18n.cpp | ||
---|---|---|
38 | This still doesn't work, did you actually test the code before uploading it? if (language.isEmpty()) { this will be true also when the variable is just set without any value (eg. LANGUAGE="" falkon) |
src/lib/plugins/qml/api/i18n/qmli18n.cpp | ||
---|---|---|
35 | This is Qt, so no getenv. You should use qEnviornmentVariableIsSet. |
src/lib/app/profilemanager.cpp | ||
---|---|---|
309 ↗ | (On Diff #39967) | const |
src/lib/data/data/browsedata.sql | ||
58 ↗ | (On Diff #39967) | In previous revision I said that I want this (enabled plugins settings in sqlite database) moved to separate revision. Or even to remove it and make it stored in QSettings, because right I don't really see a reason to have it in database. |
src/lib/history/history.cpp | ||
186 | QStringLiteral (QSL) | |
310 | It leaks here, nothing will ever delete it. HistoryEntry is meant to be passed by value. | |
329 | If this is false it will return HistoryEntry with not initialized member variables. | |
src/lib/history/history.h | ||
70 | QList<HistoryEntry*> | |
src/lib/plugins/plugins.cpp | ||
120 | just remove(0, 4), or better mid(4). | |
426 | Why is it duplicated there? | |
src/lib/plugins/qml/api/browseraction/qmlbrowseraction.h | ||
38 | What is identity? | |
98 | override | |
99 | override | |
164 | QQmlComponent * | |
180 | QQmlComponent * | |
src/lib/plugins/qml/api/extensionscheme/qmlextensionscheme.cpp | ||
32 | So this was discussed in the previous review and the outcome was that it will not work like that, and it must pass QJSValue in the signal. Am I correct? | |
src/lib/plugins/qml/api/i18n/qmli18n.cpp | ||
32 | QStringLiteral | |
33 | const + QStringLiteral | |
src/lib/plugins/qml/api/settings/qmlsettings.cpp | ||
29 | Empty destructor is not needed | |
107 | QL1S("/") -> QL1C('/') | |
src/lib/plugins/qml/api/sidebar/qmlsidebar.h | ||
121 | initialization | |
src/lib/plugins/qml/api/tabs/qmltab.cpp | ||
422 | You should be able to disconnect all with m_webTab->disconnect(this) (test first!) | |
src/lib/plugins/qml/api/userscript/qmluserscript.h | ||
29 | Some classes are exported and some not, so which way is it? It should not be needed to export any QML support classes, as there is no reason for C++ plugins to use them. The only issue might be with tests, but it doesn't seem to be used there. | |
src/lib/plugins/qml/api/windows/qmlwindows.cpp | ||
83 | If you want to show this warning then it should have the windowId that wasn't found, otherwise it's just useless. | |
src/lib/plugins/qml/api/windows/qmlwindowstate.h | ||
25 ↗ | (On Diff #39967) | This is only for the WindowState enum? In that case it would be better to create class Enums and move all these "standalone" enums there. It will also look better from QML side: instead of "Falkon.WindowState.WindowState.FullScreen" -> "Falkon.Enums.WindowState.FullScreen". |
src/lib/plugins/qml/qmlplugininterface.cpp | ||
122 | Use QQuickWidget instead. | |
src/lib/plugins/qml/qmlpluginloader.cpp | ||
25 | #if HAVE_LIBINTL because HAVE_LIBINTL is defined always. | |
29 | extra newline | |
48 | [this] { space | |
74 | QStringLiteral | |
src/lib/plugins/qml/qmlplugins.cpp | ||
67 | reason (last parameter) is QString, so it should use QStringLiteral. Or just leave it empty with QString(). | |
src/lib/plugins/qml/qmlstaticdata.cpp | ||
82 | The entire logic here is wrong too, you will never get cache hit because HistoryEntry is only ever created from the QML support code. It should use the HistoryEntry contents as key, not address. | |
src/lib/plugins/qml/qmlstaticdata.h | ||
54 | const-ref |
src/lib/plugins/qml/api/userscript/qmluserscript.h | ||
---|---|---|
29 | Yes, its exported for tests - https://phabricator.kde.org/D14775#change-lYrxE335xPKH |
src/lib/plugins/plugins.cpp | ||
---|---|---|
426 | to get the settings path m_settingsPath = DataPaths::currentProfilePath() + QL1S("/extensions"); | |
src/lib/plugins/qml/api/browseraction/qmlbrowseraction.h | ||
38 | qobject id of the button, but since id will have name collision with id (in qml) so you had suggested me to use it by name 'identity' |
config.h.cmake | ||
---|---|---|
13 | It should be #cmakedefine01 HAVE_LIBINTL. | |
src/lib/history/history.cpp | ||
330 | Same issue here, nothing will delete it. | |
src/lib/plugins/qml/api/extensionscheme/qmlextensionscheme.cpp | ||
32 | It must be requestStarted(const QJSValue &request). | |
src/lib/plugins/qml/api/history/qmlhistory.cpp | ||
45 | const HistoryEntry &entry | |
src/lib/plugins/qml/qmlplugininterface.cpp | ||
122 | This should return nullptr. Again did you test it? You should use QQuickWidget instead of QWidget::createWindowContainer. | |
src/lib/plugins/qml/qmlstaticdata.cpp | ||
84 | ? | |
src/lib/plugins/qml/qmlstaticdata.h | ||
85 | const-ref | |
90 | QSL | |
src/scripts/middleclickloader/metadata.desktop | ||
6 ↗ | (On Diff #41866) | These translations changes are unrelated and should not be here. |
src/lib/plugins/qml/api/extensionscheme/qmlextensionscheme.cpp | ||
---|---|---|
32 | Sorry, I didn't get you. Can you please elaborate? Why QJSValue? Its meant to be like the QML way and not JS function - requestStarted: { var url = request.url ... } |
src/lib/plugins/qml/api/extensionscheme/qmlextensionscheme.cpp | ||
---|---|---|
32 | We already discussed this in previous review. |
updated revision, it is probably ; ) fine now :D
src/lib/plugins/qml/qmlplugininterface.cpp | ||
---|---|---|
122 | QQuickWidget takes source and not QQmlComponent - so its not suitable to use here. |
cmake/FindLibIntl.cmake | ||
---|---|---|
7 | It should be TRUE/FALSE | |
src/lib/plugins/qml/api/events/qmlkeyevent.h | ||
68 | Why this change? | |
src/lib/plugins/qml/api/userscript/qmluserscript.cpp | ||
114 | You should probably leave the empty default case here, otherwise it will generate compile warnings. | |
src/lib/plugins/qml/qmlplugininterface.cpp | ||
122 | https://code.qt.io/cgit/qt/qtdeclarative.git/tree/src/quickwidgets/qquickwidget.h#n103 It's not documented though. |
updated revision
src/lib/plugins/qml/api/events/qmlkeyevent.h | ||
---|---|---|
68 | Because I was returning -1 when m_keyEvent is null (But I doubt that it will not be null, since user is not creating QmlKeyEvent). |
src/lib/plugins/qml/api/events/qmlkeyevent.h | ||
---|---|---|
68 | Just return 0 in that case, and yes make it quint32 again. |