backplane-import
commented
12 years ago Closed backplane-import closed 12 years ago
backplane-import
commented
12 years ago backplane-import
commented
12 years ago Thanks for this Anton.
I'm very excited about your changes, so I have indeed been working on importing them for some time now.
However, there are a number of problems, the main one of which is that the changes cover too many areas for me to simply import in one go. Even seemingly small changes may have large consequences, and so it's very risky to import them in an undifferentiated way.
For example, in your revision b38ea381 you have changed the code that detects which browser we are running in. I'm sure the code is fine, but since that could have a major impact on other code further down the line, it's a good idea for this change to have its own revision entry in the main repo.
So I'm therefore going through each of your changes and grouping them together, ready to commit as a set of patches. That way we don't get every single revision in the main repo, but we do get more than a simple commit of all changes.
As we've discussed, for the future it's important to submit patches that deal with a discrete area so that we can more easily see what is being changed. (This is not your fault -- we've been evolving how we manage revisions and patches, and it's only through the experience of bringing in your code that I've got a better idea of how to do it.)
Anyway, these changes are looking good, so please bear with me a little longer as we try to get them incorporated.
Updates
Ticket status set to Started
backplane-import
commented
12 years ago I should add, by the way, that every time I try to apply one of your revisions as a patch (or use 'hg transplant') it gets rejected. It seems that there is something awry with the EOL characters, which causes the patch to fail to synchronise with the original source. One possibility is that the although the files are stored with CRLF (to help our Windows friends!), patches seem to be applied based on just LF.
So that you can try to reproduce the problem, my set-up is as follows:
anton: an alias for a clone of 'creavenmoro-backplanejs' backplanejs: a clone of the main 'backplanejs' repo
In the backplanejs directory I generate a patch from a revision in the 'anton' repo with the following command:
hg export -R anton b38ea381f18fda98f80c5e10e1a86ad3d24e3a6f | hg qimport -n anton-sniffer -
If I then try to apply this patch with:
hg qpush
I get a single rejected hunk at line 14. If I view the .rej file I see the patch that supposed to be applied but there are additional blank lines between each line.
backplane-import
commented
12 years ago no blank lines for me, just lines from new and old files: http://skitch.com/creaven/d1gwa/sniffer.js.rej
backplane-import
commented
12 years ago The import problem seems to be caused by your editor being set to use LF by default, so when you committed code it appears you changed the files from CRLF to LF.
This would have been fine if I was taking all of your changesets in order, but since I was working through the changesets and picking bits from here and there, every single patch I tried was being rejected, because the EOL characters were always different.
The only way around this would have been to know which changeset caused the EOL characters to be changed from CRLF to LF, and then apply that patch first, but since there are many changesets that just tidy up the code, that would have taken an age.
Anyway, I've now added the Mercurial EOL extension which allows us to configure things so that whenever we extract a file from the repo it will use the EOL character for the native system. Since we're on Macs this means we'll get LF-only files. And since that matches the patches that I'm creating from your repo, then I can now apply them.
Windows users will get CRLF files, but hopefully that will be ok, if they aren't patching. We will have to decide what the default EOL format should be for the repo itself, since we have a mixture of Windows and Mac users, but we can revisit this later -- at least for now I'm finally making progress!
backplane-import
commented
12 years ago Hi Anton,
There's a problem with one set of changes, but I don't know what it is, so I'm bouncing it back to you.
To see the issue in action, do the following:
Take a clone of my personal backplanejs repo which contains the three patch branches that I am working on (i.e, your changesets):
hg clone https://markbirbeck-backplanejs.googlecode.com/hg/ markbirbeck-backplanejs
The default branch is a copy of the master repo. This build has decoration switched off for FF, and it has a test for external instance loading.
Before looking at the patch branches verify that all of this is working ok:
ant compile && ant test-compile && ant functional-test
Move to the first patch branch in the sequence:
hg update anton-housekeeping
This contains various changes you've made to the formatting, so shouldn't affect the code -- and indeed, at this point the tests all still pass correctly.
Move to the decorator refactoring patch branch:
hg update anton-decorator
Again, compile everything and run the tests, and this time you'll get failures. In order to get a full report I had to increase the Selenium RC timeout value, but that could be because my machine is running a bit slow. If you find you need to do the same, the value is at line 33 of the selenium-rc.xml file, and I set it as follows:
(I've changed this in the master repo.)
diff --git a/third-party/uxf/src/lib/sniffer.js b/third-party/uxf/src/lib/sniffer.js --- a/third-party/uxf/src/lib/sniffer.js +++ b/third-party/uxf/src/lib/sniffer.js @@ -38,7 +38,7 @@
UX.isXHTML = !!(document.xmlVersion || (document.contentType && document.contentType === "application/xhtml+xml"));UX.hasDecorationSupport = UX.isIE || UX.isFF;
UX.isQuirksMode = document.compatMode === "BackCompat"; })();
You'll find that all tests now pass except for "Test loading external instances".
RECAP
There are two problems after the refactoring:
I doubt that they are very big problems but I don't want to go any further with the integration until they are fixed.
If you create a fresh online clone (or clear your current one) and then modify the patch branches that I've created then I can just do a pull from you and then carry on with my integration.
(I have a full copy of your repo so it shouldn't be a problem if you clear your current one. Don't make any further changes to the default branch though, since that should be used to keep in sync with the master repo.)
Thanks!
backplane-import
commented
12 years ago fix attached
Attachments
decorator-fix.diff (1.1 KB)
backplane-import
commented
12 years ago Thanks Anton. The patch has been incorporated, and it fixes the 'anton-decorator' branch and the 'anton-namespace-manager' branch.
The 'anton-optimise-behavior' branch still doesn't work though. That was quite a complex merge because Frankie had also changed some of the files that you had updated, so I may have made a mistake. Anyway, it would be good if you could take a look at that branch next.
Thanks.
backplane-import
commented
12 years ago Should also add that your patch fixes the 'anton-htc-fix' branch too.
backplane-import
commented
12 years ago optimise-behavior contains only one commit, it requires all other
backplane-import
commented
12 years ago I thought that might be the case and I've been working on bringing in other changesets on top of that branch. It's a bit slow though because some of these revisions include fixes and changes for a variety of things.
Anyway...progress is being made. :)
backplane-import
commented
12 years ago Fixed in f07e8fc3b4f00461f321244a8c198b49e4d997b3 (originally e436949ff2).
Updates
Ticket status set to Fixed
Type: Review → Enhancement
Imported from backplanejs Google Code issue 96.
Main repo out of date and should add all changes from creavenmoro-backplanejs clone
Owner set to markbirbeck
Type: Review Priority: Medium