Fix #569: Merge Bramble with upstream Adobe Brackets (1.9 dev) #588
Conversation
humphd
force-pushed
the
merge-master-brackets1.9
branch
from
2469504 to
851a257
Compare
|
Out of interest, some stats:
It took me about 8 hours to deal with the conflicts between the two branches, which total +45,846 −27,697 = 18,149 LOC. |
|
This seems to run now, and I've fixed all the new eslint issues. I've also merged in the latest stuff on This needs a ton of testing before we land. |
|
One thing I see on console is about a conflict in the keybindings: |
|
@gideonthomas actually, I need some advice on fixing something. They have a new string that is messing up the localization scripts: https://github.com/adobe/brackets/blob/master/src/nls/root/strings.js#L788 Is it because of the |
|
@gideonthomas nvm, I figured it out and fixed locally. |
|
This is getting closer and closer. Some notes:
I'd love to finish this and land things this week, so if you can help me @gideonthomas, it would be great! |
|
I see three errors in the console: EDIT: I feel like the first error is the real problem and it's causing the other two. |
|
@gideonthomas I'll dig into those errors. Did you look at the copy stuff and |
|
To be honest, I'm not even clear why they install all these https://github.com/adobe/brackets/search?utf8=%E2%9C%93&q=node_modules I wonder if we can just skip it? |
|
@gideonthomas that error is due to the change here: https://github.com/mozilla/brackets/pull/584/files#diff-cedef1a0021f9b0a7a54ca7d3c53caa9 which references a string and command that weren't added, which causes this to fail: https://github.com/mozilla/brackets/pull/584/files#diff-cedef1a0021f9b0a7a54ca7d3c53caa9R104. cc @peiying16. I've pushed a patch that fixes it. When we aren't adding commands that need UI, we should prefer I've also fixed the same error in the bramble-move-file extension. |
|
Ah, nice catch @humphd! Yeah I'm looking through the copy stuff right now (it's hard to find where some of these are being used). |
|
I'm not sure I understand why Move File isn't working though...it has the string defined here: https://github.com/mozilla/brackets/blob/master/locales/en-US/editor.properties#L184 |
|
These commits might point at things:
|
|
Probably it's a |
Don't run npm-install grunt task
|
@humphd fyi, doesn't look like we can use |
Gruntfile.js
Outdated
| @@ -660,7 +660,7 @@ module.exports = function (grunt) { | |||
| /*'cssmin',*/ | |||
| /*'uglify',*/ | |||
| 'copy:dist', | |||
| 'npm-install', | |||
| // 'npm-install', | |||
There was a problem hiding this comment.
it's git so you can always get this line back. If this isn't a work-in-progress PR, might as well just remove the line (and the commented lines above maybe?)
|
This patch also fixes:
@humphd we have Cut/Copy/Paste enabled in the context menu. Cut and Copy work perfectly for me but paste doesn't. Interestingly, paste in general (even if we don't use the context menu) doesn't work - which is pretty bad. Other than this, I tested this and couldn't find other issues. |
|
Thanks for testing this so well! I'll do more on this tomorrow, but I had to research the paste issue. I'm so glad you caught that, I totally missed it. So it seems they switched over to use I think for the purposes of getting this merge to happen, we should revert to ignoring those 3 key events, and let the browser handle them. Then we should consider filing a new bug to implement this with something like https://clipboardjs.com/. @Pomax, have you got a handle on where things are these days on cross-browser clipboard handling? I'll try to finish this up tomorrow and land it. |
|
Related question: since |
relies on document.execCommand() which doesn't work well (esp. for paste) in browsers. See adobe#12674. This reverts commit d93ccc5.
|
I had not heard of (I'm sure it works brilliant in an electron wrapped because of V8, but not so much on the web) |
|
@Pomax not for Also, I tracked down the final console error that this merge is causing: In #474 we stole Which one should I take? |
|
I've filed #590 to do this process again down the road. |
|
@gideonthomas I've just removed our totally-unguessable I think I'm done here. If you're OK, I'd like to merge. Let me know. |
|
@flukeout, @Pomax, etc. a note about this. If you see students having troubles, or other community people, here is how you update to get this patch locally (assuming Mozilla's Brackets repo is named |
|
For future reference, this introduces a non-fatal build error installing the iltorb module for grunt-contrib-compress: The error can safely be ignored, since this module is optional. We should also fix our travis setup to get around this: https://docs.travis-ci.com/user/languages/javascript-with-nodejs#Node.js-v4-(or-io.js-v3)-compiler-requirements |
|
Filed #592 on the build error. |
mozilla/brackets: 3754f1a (Wed Feb 8, 2017)
adobe/brackets: e9399ba (Sun Jan 29, 2017)
This is almost done. I've fixed all the merge conflicts, and am starting on fixing regressions. Once I'm done we'll need to test it for a while, since I have no idea how badly this will break things.