Skip to content

api: Support assigning qualified qmlelem to a property - #18

Merged
stephenmdangelo merged 2 commits into
qmlweb:masterfrom
stephenmdangelo:qualified_qmlelem_assignment
Jan 3, 2017
Merged

api: Support assigning qualified qmlelem to a property#18
stephenmdangelo merged 2 commits into
qmlweb:masterfrom
stephenmdangelo:qualified_qmlelem_assignment

Conversation

@stephenmdangelo

Copy link
Copy Markdown
Member

e.g.

import QtQuick 2.5 as QQ

Repeater {
delegate: QQ.Rectangle {}
}

@stephenmdangelo
stephenmdangelo force-pushed the qualified_qmlelem_assignment branch 2 times, most recently from beb600d to da3ec0d Compare July 5, 2016 16:31

@ChALkeR ChALkeR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

qmlelem format changed in 6e5bf1c, and those strings are now parsed deeper.

This PR should adapt to those changes after a rebase.
Also, it shouldn't remove the existing testcase for property Item item: Item {}.

Comment thread tests/qml/Properties.qml.json Outdated
[
"qmlelem",
"Item",
"QQ.Item",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not the proper format for qmlelem. See ImportsNamed.qml.json#L27-L32 for how qualified qmlelem should be encoded. You could also test with longer paths. e.g. Abc.Def.Rectangle.

Comment thread tests/qml/Properties.qml Outdated
property var foo: {}
property var bar: []
property Item item: Item {}
property Item item: QQ.Item {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be added as a new line, and should not replace the current property Item item: Item {} test.

Comment thread tests/qml/Properties.qml Outdated
import QtQuick 2.5 as QQ

Rectangle {
QQ.Rectangle {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unneeded change.

Comment thread tests/qml/Properties.qml Outdated
@@ -1,6 +1,6 @@
import QtQuick 2.5
import QtQuick 2.5 as QQ

@ChALkeR ChALkeR Dec 26, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The original line should be present to keep the existing testcase (see comment below).
Perhaps use some other import, not QtQuick, and add it to the second line?

Comment thread tests/qml/Properties.qml.json Outdated
[
"qmlelem",
"Rectangle",
"QQ.Rectangle",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This has changed and will be ["dot", "QQ", "Rectangle"], after a rebase.
Note that this line shouldn't even be present here, as RectangleQQ.Rectangle is an unrelated change on this line.

@ChALkeR

ChALkeR commented Dec 26, 2016

Copy link
Copy Markdown
Member

@stephenmdangelo, sorry for not reviewing at this earlier. In general, I'm in favor of this PR (and thanks for doing this!), but it requires some changes before landing. See the review above.

@stephenmdangelo
stephenmdangelo force-pushed the qualified_qmlelem_assignment branch 2 times, most recently from 6558d26 to dee5696 Compare January 2, 2017 18:30
@stephenmdangelo

stephenmdangelo commented Jan 2, 2017

Copy link
Copy Markdown
Member Author

@ChALkeR So I've cleaned up the Properties.json test (as well, I added a change to tape.js to fix the tests failing if, say, I was editing in vi).

I think I'm generating the dot expression as you're suggesting.

Though I find it odd that the expr as returned by maybe_assign is of the form:

[
  "dot",
  "DOM",
  [
    "name",
    "DOM"
  ],
  "Div"
]

But a non-assigned qmlelem (as in ImportsNamed.qml) omits the name array:

[
   "dot",
   "DOM",
   "Div"
]

So I went with the latter form. Is this correct? If so, why don't we use the name format, or why does that format get returned by maybe_assign?

Comment thread tests/tape.js
for (const file of fs.readdirSync(dir)) {
const filePath = path.join(dir, file);
if (file[0] === ".") {
continue;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: should be above const filePath.

@ChALkeR ChALkeR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with a nit.

@ChALkeR

ChALkeR commented Jan 2, 2017

Copy link
Copy Markdown
Member

I will take a look at maybe_assign a bit later, though. =)

@stephenmdangelo
stephenmdangelo force-pushed the qualified_qmlelem_assignment branch from dee5696 to 673e84f Compare January 3, 2017 13:36
e.g.

import QtQuick 2.5 as QQ

Repeater {
  delegate: QQ.Rectangle {}
}

PR-URL: qmlweb#18
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Some editors (i.e. vi) may put temporary files into the tests directory.

Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@stephenmdangelo
stephenmdangelo force-pushed the qualified_qmlelem_assignment branch from 673e84f to 83b9686 Compare January 3, 2017 13:39
@stephenmdangelo
stephenmdangelo merged commit 83b9686 into qmlweb:master Jan 3, 2017
@stephenmdangelo
stephenmdangelo deleted the qualified_qmlelem_assignment branch January 3, 2017 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants