Add QML Extensions API
ClosedPublic

Authored by anmolgautam on Aug 12 2018, 5:46 PM.

Details

Summary

Add QML Extensions API

Diff Detail

Repository
R875 Falkon
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
  • improved i18n
drosca added inline comments.Aug 17 2018, 3:33 PM
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.

anmolgautam added inline comments.Aug 17 2018, 4:33 PM
src/lib/plugins/qml/api/i18n/qmli18n.cpp
34

And if its set before then it shouldn't set it to QLocale::system?

drosca added inline comments.Aug 17 2018, 6:46 PM
src/lib/plugins/qml/api/i18n/qmli18n.cpp
34

It should always set it, this is meant to force language that we want.
It only matters for restoring the env variable, if it was previously set, restore the value, otherwise unset the variable.

  • improved i18n
drosca added inline comments.Aug 18 2018, 5:44 AM
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)

  • revised i18n
drosca added inline comments.Aug 18 2018, 12:01 PM
src/lib/plugins/qml/api/i18n/qmli18n.cpp
35

This is Qt, so no getenv. You should use qEnviornmentVariableIsSet.

  • improved i18n
drosca requested changes to this revision.Sep 2 2018, 8:32 PM
drosca added inline comments.
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
100

just remove(0, 4), or better mid(4).

423

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

This revision now requires changes to proceed.Sep 2 2018, 8:32 PM
anmolgautam added inline comments.Sep 9 2018, 9:07 PM
src/lib/plugins/qml/api/userscript/qmluserscript.h
29
  • updated revision
anmolgautam marked 28 inline comments as done.Sep 17 2018, 8:31 PM
  • added missing QSL
anmolgautam marked 3 inline comments as done.Sep 17 2018, 8:44 PM
anmolgautam added inline comments.
src/lib/plugins/plugins.cpp
423

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'

I have corrected it, it should be fine now :)

drosca requested changes to this revision.Sep 21 2018, 7:48 AM
drosca added inline comments.
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.

This revision now requires changes to proceed.Sep 21 2018, 7:48 AM
anmolgautam added inline comments.Sep 23 2018, 10:40 AM
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
    ...
}
drosca added inline comments.Sep 23 2018, 10:50 AM
src/lib/plugins/qml/api/extensionscheme/qmlextensionscheme.cpp
32

We already discussed this in previous review.
Unless it is passed as QJSValue (from QJSEngine::newQObject) to QML engine it won't be correctly released.

anmolgautam marked 9 inline comments as done and an inline comment as not done.

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.

drosca added inline comments.Sep 24 2018, 8:10 AM
cmake/FindLibIntl.cmake
6

It should be TRUE/FALSE

src/lib/plugins/qml/api/events/qmlkeyevent.h
67

Why this change?

src/lib/plugins/qml/api/userscript/qmluserscript.cpp
113 ↗(On Diff #42017)

You should probably leave the empty default case here, otherwise it will generate compile warnings.

src/lib/plugins/qml/qmlplugininterface.cpp
122
anmolgautam marked 6 inline comments as done.

updated revision

src/lib/plugins/qml/api/events/qmlkeyevent.h
67

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).
Should I remove the check and make it quint32?

drosca added inline comments.Sep 24 2018, 2:18 PM
src/lib/plugins/qml/api/events/qmlkeyevent.h
67

Just return 0 in that case, and yes make it quint32 again.

anmolgautam marked an inline comment as done.

updated revision

drosca accepted this revision.Sep 28 2018, 10:06 AM
This revision is now accepted and ready to land.Sep 28 2018, 10:06 AM
This revision was automatically updated to reflect the committed changes.