core-ai-bot
commented
3 years ago 
When I just tried this branch, I noticed that Debug > Open preferences file no-ops when you're not in Split View and none of the files is already open.
Open core-ai-bot opened 3 years ago
core-ai-bot
commented
3 years ago
When I just tried this branch, I noticed that Debug > Open preferences file no-ops when you're not in Split View and none of the files is already open.
core-ai-bot
commented
3 years ago 
@MarcelGerber That was quick :smile: Thanks for starting to test this! I just checked. My bad. I pushed some bad change in one of the last commits that is causing this. For now to see what it looks like, be in the split view and then try Debug > Open Preferences File. I will check the code and push in a fix. Thanks!
core-ai-bot
commented
3 years ago
@MarcelGerber I have addressed the code review comments and updated the PR. Also I have fixed the bug with opening defaultPreferences and brackets.json in split view. Could you take a look at it and let me know if anything else needs to be changed?
core-ai-bot
commented
3 years ago
@MarcelGerber@abose@sprintr I have made the suggested changes. This is up for review.
core-ai-bot
commented
3 years ago 
There are too many commits in this PR, maybe squash some of them for good measure :)
core-ai-bot
commented
3 years ago
We should make sure nothing can change the contents of this read-only file, no core features (like Replace, Replace in Files) and presumably extensions, too
core-ai-bot
commented
3 years ago
@MarcelGerber It is a good idea to make sure none of the other features can modify this file. Opening the file in read only mode was an attempt to make sure this file does not get modified by other features. We could also make it read only. (For this I will have to add chmod to our app shell file system api). Does adding a chmod step alone, help in making sure none of the other features will be able to make a change to this file? I do not have any other ideas on how to make sure other features don't make any change to defaultPreferences. So any ideas in this area are welcome :smile:
core-ai-bot
commented
3 years ago
@abose I will do an git -i rebase once you guys are OK with the PR.
core-ai-bot
commented
3 years ago
I just tried using "Replace" in this file and you can indeed change the file contents that way.
The best solution would probably be a read-only flag for the Document, but I suppose it'd be a rather disruptive change at this point. Thus, I think it's ok as it is right now. At least the most obvious ways cannot be used to alter the file.
core-ai-bot
commented
3 years ago
@nethip I noticed you use a mixture of _type and prefType with no clear differentiation.
I guess we can improve that.
core-ai-bot
commented
3 years ago
This code is quite complex for this not-that-complex task. We should really try to make it more comprehensible.
core-ai-bot
commented
3 years ago
@MarcelGerber I will address all the code review comments that you have proposed(Thanks!).
When I started with this task I have written barely 50-100 lines to get the task done but it was great amount of unit testing that made me end up with ~800 lines of code. The following are some of the reasons why the code is really looking complicated.
keys and initial object. I found out that, in some extensions, authors are sometimes declaring default values in the initial object itself. JSON.stringifyIf there are some things, like variable names, that make the code not comprehensible, which do not fall in the above categories, please call them out so thatI can work on those areas.
core-ai-bot
commented
3 years ago
@MarcelGerber I have incorporated your code review comments into this PR and this is up for review again.
core-ai-bot
commented
3 years ago core-ai-bot
commented
3 years ago core-ai-bot
commented
3 years ago 
sorry that I am late to this PR but I tried it out yesterday and wanted to make a suggestion that I don't think has been discussed yet, but if it has, I apologize in advance ;)
I think the editable preferences file (brackets.json) should be on the left not the right. It fits in better with the left-to-right workflow most Western languages have and it makes much more sense to me that I am editing on the left and glancing to my right to look at the reference material. In all 3 of my computer workstations, I have multiple monitors with the primary one directly in front of me and the 2nd monitor to it's right in my peripheral vision. This is a very similar concept to the 2 pane setup of this PR.
core-ai-bot
commented
3 years ago 
:+1: for@mackenza's suggestion. Another think I would like is to close defaultPreferences.json once brackets.json is closed, and restore the layout scheme to what it was before opening the preferences.
core-ai-bot
commented
3 years ago
@mackenza Sure! How about a preference?
@sprintr We could do that too.
core-ai-bot
commented
3 years ago
@nethip It would be nice if both of them have preferences. defaultPreferences.json should not close for someone who opened Preferences just to read them.
core-ai-bot
commented
3 years ago
let's go easy on this, though... it doesn't need to be a Cadillac feature. Preferences are typically changed on first install and then rarely changed after that. The PR is a good idea, but complicating it with all kinds of use cases around opening/closing and restoring views seems a bit... too much imo.
think bikeshed ;)
core-ai-bot
commented
3 years ago
@sprintr I have changed the for() to forEach(). Also now I am sorting the keys.
@mackenza This PR is now updated with a new preference to display the preferences either on the right side or the left side. Please check it out and let me know if this looks fine.
@MarcelGerber Let me know if the changes look good to you. I could then rebase this entire branch to a very limited no of commits before we merge this to master.
core-ai-bot
commented
3 years ago
@nethip I get an error trying to open up the split view with your last changes:
Console says:
Uncaught TypeError: Cannot read property 'removeView' of null in view/MainViewManager.js:810
core-ai-bot
commented
3 years ago core-ai-bot
commented
3 years ago
@mackenza@sprintr Thanks a lot guys for trying out this PR and letting me know about the issue. This kind of crept in with the addition of the new preference to display the user preferences either in the first-pane or second-pane. I now have updated my branch with the fix. Also I have added default values of closeTags. Could you take the latest changes and checkout the feature and let me know if you guys see anything fishy. I really appreciate you guys taking some time in trying this feature out. Thanks!
core-ai-bot
commented
3 years ago
@nethip :cool: Works just as expected. :+1: for making the new layout default. The opening/closing/restoring layout scheme issue can be skipped for the moment, it looks more like an extension idea.
core-ai-bot
commented
3 years ago
Since this needs to make into 1.4 we would want to send the strings for translations. I would like to know if there any parts of this PR that needs to be worked on. Or are we good to go with this change, in which case I will go ahead and merge this change.
@MarcelGerber@sprintr@abose@mackenza@
core-ai-bot
commented
3 years ago 
Currently, the comments at the beginning of defaultPreferences.json looks untidy when opened in split view,
I would suggest a cleaner
core-ai-bot
commented
3 years ago
@nethip Everything else looks good from my side. :+1:
I have run unit tests and scenario tests and all have passed.
I can merge it once the comments are addressed.
core-ai-bot
commented
3 years ago
@nethip I am really not crazy about the preference name for whether the editable window is left or right.
alwaysOpenUserPrefsinSecondPane is a real mouthful and semantically questionable. A simple fix might be to remove the "always". It's not needed. We don't have it in the other preference related to this: openPrefsInSplitView so I feel it's redundant.
So, in the interest of getting this feature merged, I propose we simply go with:
openUserPrefsInSecondPane (notice I also camel-cased the I in In
core-ai-bot
commented
3 years ago
@mackenza I agree with you on changing the preference name. Just did it. Also changed the description variable name along with other string changes.
@rroshan1 I incorporated your changes as well.
With this PR, upon choosing
Debug > Open Preferences file, preferences filebrackets.jsonwill open in the second pane of split view while the default preferences will load in the left-pane. The default preferences are generated on the fly by iterating through all valid preferences and writing into the default preferences file,defaultSettings.jsoncreated insidebracketspreferences folder. Here is the work card for the sameThis is the sample screenshot of how the split view looks like.
I am going to add unit tests for these. Meanwhile I would like to get some eyes on the changes and also make some code changes based on the review comments.
@sprintr@MarcelGerber@abose Please review this when you get a chance.Thanks!cc
@ryanstewartPending Tasks
Add unit tests: Adding unit tests to this PR is not straight forward as we are using shell apis for this PR and they are not working with our unit test framework. I have added a waffle card to track this specific task.nethip included the following code: https://github.com/adobe/brackets/pull/11307/commits