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.

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

@pavelvasev merge: Merge recursive props #233

Merged
merged 5 commits into from Jun 13, 2016
Merged

Conversation

henrikrudstrom
Copy link
Member

merged recursive properties from @pavelvasev branch
@ChALkeR

property.binding.compile();
property.update();
if (property.needsUpdate)
+ property.update();
Copy link
Member

Choose a reason for hiding this comment

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

Stray +.

@ChALkeR
Copy link
Member

ChALkeR commented May 6, 2016

@henrikrudstrom Thanks! Will take a look at this soon.
I will probably just force-push this branch to fix minor nits, ok?

@henrikrudstrom
Copy link
Member Author

Thats totally fine!
Fri 6 May 2016 kl. 17:00 skrev Сковорода Никита Андреевич <
notifications@github.com>:

@henrikrudstrom https://github.com/henrikrudstrom Thanks! Will take a
look at this soon.
I will probably just force-push this branch to fix minor nits, ok?


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#233 (comment)

@ChALkeR
Copy link
Member

ChALkeR commented May 8, 2016

595a29f got missed, I will add that on top.

ChALkeR pushed a commit that referenced this pull request May 8, 2016
ChALkeR pushed a commit that referenced this pull request May 8, 2016
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
ChALkeR pushed a commit that referenced this pull request May 8, 2016
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@ChALkeR ChALkeR mentioned this pull request May 8, 2016
62 tasks
@ChALkeR
Copy link
Member

ChALkeR commented May 8, 2016

I am going to rename evaluatingPropertyTop back to to evaluatingProperty, that was an unneeded change.

Moreover, this breaks code in QMLEngine.js that is dependent on evaluatingProperty.

Update: done.

ChALkeR pushed a commit that referenced this pull request May 8, 2016
ChALkeR pushed a commit that referenced this pull request May 8, 2016
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
ChALkeR pushed a commit that referenced this pull request May 8, 2016
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@ChALkeR
Copy link
Member

ChALkeR commented May 8, 2016

Btw, this merges 0aab767, 09b8243, 0e80a6e, 595a29f.

ChALkeR pushed a commit that referenced this pull request May 8, 2016
ChALkeR pushed a commit that referenced this pull request May 8, 2016
ChALkeR pushed a commit that referenced this pull request May 8, 2016
ChALkeR pushed a commit that referenced this pull request May 8, 2016
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
ChALkeR pushed a commit that referenced this pull request May 8, 2016
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
if (i == "id" || i == "$class") { // keep them
item[i] = value;
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

@pavelvasev, what's going on here? This contradicts the check below, i == 'id' below never fires.

@ChALkeR ChALkeR added the merge label May 8, 2016
@ChALkeR
Copy link
Member

ChALkeR commented May 9, 2016

Ah, and tracking: #32.

@ChALkeR ChALkeR changed the title Merge recursive props @pavelvasev merge: Merge recursive props Jun 9, 2016
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
ChALkeR pushed a commit that referenced this pull request Jun 13, 2016
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@ChALkeR
Copy link
Member

ChALkeR commented Jun 13, 2016

Merging #248 broke this.

pavelvasev and others added 5 commits June 13, 2016 19:56
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
PR-URL: #233
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@ChALkeR
Copy link
Member

ChALkeR commented Jun 13, 2016

Ok, I fixed the rebase.

The last two commits LGTM, the first three seem fine, I will just merge them for now — we can fix things later after the merges are complete =).

@ChALkeR ChALkeR merged commit f8e5e7a into master Jun 13, 2016
@ChALkeR ChALkeR deleted the merge-pv-recursive-props branch June 13, 2016 20:29
@stephenmdangelo
Copy link
Member

@ChALkeR So, I've run into a use-case where this breaks geometry updating, as the recursion checks within update*Geometry cause the methods to return before properly applying the appropriate updates. Reverting these commits fixes it. I haven't had a chance to pin-point it directly, yet.

I'm guessing it's related to the forced update*Geometry calls at the end of $initializePropertyBindings. It seems to only call them if they are internal connections between those geometry-related property names and the geometry of the item with those properties, which won't take into account places where anchors or geometry-related properties refer to properties on other items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants