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.

Sign up for GitHub

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

Jump to bottom

Kde master ongoing merge — rebased #57

Merged
merged 6 commits into from Mar 5, 2016
Merged

Kde master ongoing merge — rebased #57

merged 6 commits into from Mar 5, 2016

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Feb 20, 2016

This is a rebased version of #28.

Merged commits: 79a05ff, f401c53, 7dad8e8, 44bea45.

@ChALkeR ChALkeR added the merge label Feb 20, 2016
@ChALkeR ChALkeR mentioned this pull request Feb 20, 2016
Merged
@ChALkeR ChALkeR force-pushed the kde-master-ongoing-merge-rebased branch from eac51c9 to f16b8c6 Compare February 20, 2016 17:14
@ChALkeR
Copy link
Member Author

ChALkeR commented Feb 20, 2016

/cc @akreuzkamp

@ChALkeR ChALkeR force-pushed the kde-master-ongoing-merge-rebased branch from 0dafc6c to 25beddf Compare February 20, 2016 17:51
@ChALkeR ChALkeR mentioned this pull request Feb 20, 2016
Closed
27 tasks
@ChALkeR ChALkeR force-pushed the kde-master-ongoing-merge-rebased branch from 25beddf to 912c819 Compare February 20, 2016 20:06
@ChALkeR
Copy link
Member Author

ChALkeR commented Feb 20, 2016

Actually, 9ebf5d8 is the first failing commit, something is terribly wrong with it.

@akreuzkamp akreuzkamp force-pushed the kde-master-ongoing-merge-rebased branch from 912c819 to 383c110 Compare February 21, 2016 14:00
@ChALkeR
Copy link
Member Author

ChALkeR commented Feb 21, 2016

@akreuzkamp Thanks for the update!
adacdf8 looks fine now, but 9717c63 is breaking the build.
I'm taking a look at it now.

@ChALkeR ChALkeR force-pushed the kde-master-ongoing-merge-rebased branch from 383c110 to 7bbea62 Compare February 21, 2016 14:54
@ChALkeR
Copy link
Member Author

ChALkeR commented Feb 21, 2016

/cc @akreuzkamp, @Plaristote, @pavelvasev — PTAL.

@ChALkeR ChALkeR mentioned this pull request Feb 21, 2016
Merged
@Plaristote
Copy link
Member

We may want to write some test to make this work. I've checked the bindingArray commit, and it doesn't work with me: an array that uses bound values just gets evaluated as undefined with me.

I've tested ce1c580, it LGTM.
I'm not sure how to check b5a33b8.
The array binding however doesn't seem to work.

@Plaristote Plaristote mentioned this pull request Feb 25, 2016
Merged
@Plaristote
Copy link
Member

I was wrong about the array binding. My test environment must've had issues: I wrote a test to check it, and it does work.
Check PR #73 to see the test. It currently breaks, and that's normal: it works if it is rebased on kde-master-ongoing-merge-rebased... so it'll be fine once this is merged in master.

@henrikrudstrom
Copy link
Member

looks hard to test b5a33b8 without an actual webchannel to test with.
added a test for ce1c580 but it doesnt seem to report the correct line number.

@ChALkeR ChALkeR force-pushed the kde-master-ongoing-merge-rebased branch from 271d331 to 8c764b1 Compare February 29, 2016 22:31
@ChALkeR
Copy link
Member Author

ChALkeR commented Feb 29, 2016

Squashed the test and rebased.

@ChALkeR ChALkeR force-pushed the kde-master-ongoing-merge-rebased branch from 8c764b1 to fd51eb6 Compare February 29, 2016 22:34
@ChALkeR
Copy link
Member Author

ChALkeR commented Feb 29, 2016

Fixed the test syntax.

@ChALkeR
Copy link
Member Author

ChALkeR commented Feb 29, 2016

@pavelvasev It looks like the line starts with 0 in your code, while @henrikrudstrom expects it to start with 1. That's what fails the test. How should this behave?

@henrikrudstrom henrikrudstrom force-pushed the kde-master-ongoing-merge-rebased branch from 4fcdea5 to 4c6ba67 Compare March 2, 2016 19:29
@henrikrudstrom
Copy link
Member

@ChALkeR @Plaristote @akreuzkamp
shall we?

@ChALkeR
Copy link
Member Author

ChALkeR commented Mar 3, 2016

Ok, I'm going to split this today and merge some of those commits.

@ChALkeR
Copy link
Member Author

ChALkeR commented Mar 5, 2016

Actually, it doesn't split without dependencies conflicts.

The new test looks good to me.
The parse error and the parse error fix also look good to me.
The parser fixes are covered by the new test in #73, so rubber-stamp LGTM.
«Add basic support for QtWebChannel.» just adds some hooks that seem required for some usecases, and that should not break anything. Rubber-stamp LGTM.

I'm going to merge this.

akreuzkamp and others added 2 commits March 5, 2016 21:47
@akreuzkamp @ChALkeR
Add basic support for QtWebChannel.
996deaa 
This commit adds support for synchronising property changes back to an
remote server. This enables a custom implementation of QtWebChannel to
integrate with QmlWeb.

Basically any protocol is possible.

PR-URL: #57
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@pavelvasev @ChALkeR
On parse error show extraxt of related source code
4e5cb81 
With this patch the parser will add a small extract of the source code
to the parsing exception message, where the error occured.

PR-URL: #57
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
igosoft pushed a commit that referenced this pull request Mar 6, 2016
@henrikrudstrom @igosoft
tests: added parse error test
34a453d 
PR-URL: #57
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@ChALkeR ChALkeR deleted the kde-master-ongoing-merge-rebased branch March 13, 2016 05:25
ChALkeR pushed a commit to qmlweb/qmlweb-parser that referenced this pull request May 2, 2016
@pavelvasev @ChALkeR
On parse error show extraxt of related source code
cee968b 
With this patch the parser will add a small extract of the source code
to the parsing exception message, where the error occured.

PR-URL: qmlweb/qmlweb#57
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
ChALkeR pushed a commit to qmlweb/qmlweb-parser that referenced this pull request May 2, 2016
@akreuzkamp @ChALkeR
Parser: Allow arrays with bindings.
8924363 
This patch makes the parser differentiate between:
- Simple Arrays
- Arrays with bindings in it
- Lists of qml elements

The first and the last are returned as arrays, the second as binding.
Lists that contain both bindings and qml elements are not allowed by
QML.

PR-URL: qmlweb/qmlweb#57
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
ChALkeR pushed a commit to qmlweb/qmlweb-parser that referenced this pull request May 2, 2016
@akreuzkamp @ChALkeR
Fix parsing of arrays containing bindings.
bda5b8c 
Prior it tried to traverse the whole binding, which would work for
*some* bindings (i.e. those just referencing a simple variable), but
would'nt for more complex ones, because of missing walkers for all
possible types of tokens.
Now it uses bindout() to directly create a binding from the src.

PR-URL: qmlweb/qmlweb#57
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
ChALkeR pushed a commit to qmlweb/qmlweb-parser that referenced this pull request May 2, 2016
@henrikrudstrom @ChALkeR
parser: line numbers start at one for error messages
f2fac02 
PR-URL: qmlweb/qmlweb#57
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ChALkeR @Plaristote @henrikrudstrom @akreuzkamp @pavelvasev