Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Addresses #12456 #12521

Merged
merged 9 commits into from
Aug 10, 2016
Merged

Addresses #12456 #12521

merged 9 commits into from
Aug 10, 2016

Conversation

thehogfather
Copy link
Contributor

Addresses issue where code folding triangles in the gutter were sometimes not correctly updated after deleting lines above a folded region.

… not correctly updated after deleting lines above a folded region.
@abose
Copy link
Contributor

abose commented Jul 1, 2016

Tagging @swmitra

handled case in xml type files  where the start of a collapsible region can span two lines.
@ficristo
Copy link
Collaborator

I've looked a bit at this.
I was able to reproduce the issue (trying to delete a line of code above a folded region) and indeed this PR fixes it.
@thehogfather can you add a test?

I've noticed a small issue: if collapse a folded region and then expand it, the code folding triangles are not updated until you move the cursor.
fold

I think this is a minimal fix to solve the issue, but the right fix seems to me that we should update all the code of the code folding addon of CodeMirror.

@thehogfather
Copy link
Contributor Author

@ficristo I will have a go at adding some more tests.

Could you clarify what you mean by 'update all the code of the code folding addon of CodeMirror'?

@ficristo
Copy link
Collaborator

If I'm not mistaken, the foldcode.js and foldgutter.js are a port of the CodeMirror ones adapted to work in Brackets. I think we should backport all the changes that are in CodeMirror (better in another PR).

@thehogfather
Copy link
Contributor Author

Ah yes - better a different PR. There are lots of subtle differences, mainly to do with the ability to persist and restore fold states.

@thehogfather
Copy link
Contributor Author

Changes ready for another review.
Main changes are updates to tests which now include html files as well as js.

});

if (force === "fold") {
delete range.cleared;
cm._lineFolds[pos.line] = range;
// in some cases such as in xml style files, the start of line folds can span multiple lines
// render a gutter marker for both the beginning and end of the line
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use capital letter at the start of a sentence. This needs some dots too.

@ficristo
Copy link
Collaborator

@thehogfather thank you a lot!
Could you merge / rebase with master? We have updated CodeMirror, so I would like to test with the new version.
Meanwhile I left some comments.

@ficristo
Copy link
Collaborator

If the newlines I mentioned are necessary as the globals to make testing easier, then is fine to leave them.

@ficristo
Copy link
Collaborator

@thehogfather could you take a look at the test failures?

So now when a tag span on multiple lines you have two code folding triangles. I'm not an UI / UX expert so I'm not sure is actually a good or bad idea.
html-foldable

/cc @marcelgerber

@thehogfather
Copy link
Contributor Author

@ficristo regarding the folding triangles, I think this is fine. A better design might be to hide the opening tag when the range is collapsed or to only show the fold triangles at the start of the folded/foldable range (so in your example we dont show line 25 as foldable - only line 29). This is however a little tricky - would need a rewrite of the xml-range finder. We are currently using whatever version is from code-mirror. The same workaround is present in code-mirror. I think it is ok as it doesn't reduce functionality nor does it put the editor in an unusable state.

@ficristo ficristo added this to the Release 1.8 milestone Aug 2, 2016
@redmunds
Copy link
Contributor

redmunds commented Aug 3, 2016

Another option would be to collapse both tag attributes and tag contents something like this:

tag-collapse2

@ficristo
Copy link
Collaborator

ficristo commented Aug 8, 2016

I still see some test failures on the latest commit.

@thehogfather
Copy link
Contributor Author

@ficristo would you kindly point out the failing test if possible? I typically run the extension test as opposed to 'All tests' so I might have missed something. I've just run the tests again and all are passing. Note that the last commits need a full reload of brackets so that the changes in code mirror are reflected in the tests.

@thehogfather
Copy link
Contributor Author

@redmunds interesting idea - I'd however question what happens to the fold mark in the gutter when either of the collapsed ranges in the editor is clicked. They should probably be treated as two independent ranges (as displayed in the editor) and marked as such in the gutter with two fold triangles when open.

@ficristo
Copy link
Collaborator

ficristo commented Aug 9, 2016

Since the behaviour match the CodeMirror one I'm going to take this as is and I'll open a new issue to discuss more the UI / UX.
I've run multiple times the tests and seems there are some issues with the focus of the test window: when I give the focus on it all the tests pass.

@thehogfather can you squash a bit the commits?

@thehogfather
Copy link
Contributor Author

@ficristo not sure how much more I can squash this. I have tried without much success. Maybe whoever merges on github can use the Squash and merge button? Alternatively I would probably create a new PR with all the changes if that is preferable.

@ficristo ficristo merged commit a8fe878 into adobe:master Aug 10, 2016
@ficristo
Copy link
Collaborator

@thehogfather thank you.
I've merged as is (I didn't want to mess with github)

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

Successfully merging this pull request may close these issues.

4 participants