Fix #9363: Code hints popup dismisses prematurely

[TomMalbran]

I moved the logic to restore the collapsed files to the template, so that we don't need to toggle them after the results UI is rendered.

[dangoor]

Just to get the linkage: this is for #9363

[redmunds]

@TomMalbran I can only get problem to reproduce in a slightly stricter version of @jasonsanjose's recipe:

1. When Find In... results are open, collapse the results for at least 1 file
2. Open code hints by typing (i.e. doesn't happen with Ctrl-space)
3. Observe hints appear briefly, then disappear

I confirmed that you PR fixes this case.

Is this what you are seeing, or is there another way to reproduce problem?

[redmunds]

It doesn't seem necessary to have mutually exclusive expanded and collapsed classes since you can check for the presence versus absence of a single class.

I can only find a single place the collapsed class is used in brackets.less:

.search-results .disclosure-triangle,
#problems-panel .disclosure-triangle {
    .jstree-sprite;
    display: inline-block;
    &.expanded {
        // Unfortunately, the way jsTree sprites are aligned within their 18px boxes doesn't look good in
        // other contexts, so we need some tweaks here instead of straight multiples of @jstree-sprite-size
        background-position: 7px 5px;
        -webkit-transform: translateZ(0) rotate(90deg);
    }
    &.collapsed {
        background-position: 7px 5px;
    }
}

background-position is the same for both cases, so this should be simplified to removes the reference to the collapsed class:

.search-results .disclosure-triangle,
#problems-panel .disclosure-triangle {
    .jstree-sprite;
    display: inline-block;
    background-position: 7px 5px;
    &.expanded {
        // Unfortunately, the way jsTree sprites are aligned within their 18px boxes doesn't look good in
        // other contexts, so we need some tweaks here instead of straight multiples of @jstree-sprite-size
        -webkit-transform: translateZ(0) rotate(90deg);
    }
}

[TomMalbran]

We could and we can remove the toggle classes for the expanded class.

[redmunds]

Done with review.

[TomMalbran]

@redmunds Yes. That is the way to reproduce it. The issue here is that the code hints pop up, then the search results list is re-rendered, and all the files are open. So to close them we are triggering a click event on every file which was previously collapsed. That then closes the code hints, since we triggered a click event.

The fix is simply to just render the collapsed items as hidden so that we don't need to hide them later.

[redmunds]

This selector is for an element that has the not-editor class applied twice -- that's not right is it?

[TomMalbran]

This shouldn't be here. I was testing another bug and forgot to remove it, and didn't noticed it before committing.

BTW, is not for an element which has the class applied twice it works for any element that has that class, but by adding the class twice or more in a selector you increase the specificity of the selector, making the properties overwrite the ones applied in other selectors without using !important.

[redmunds]

With latest code, I'm seeing this:

1. Do a Find in Files
2. Collapse results for one of the files
3. Make an edit to current file and Save
   (not sure if it's required, but I edited same file as was collapsed)

Results:
Collapsed file is expanded

Do you still need expanded class to save state?

[TomMalbran]

@redmunds I was able to reproduce that by:

1. Do a Find in Files for "Copyright"
2. Collapse the results for the current file
3. Remove the word "Copyright"
4. Re-add the word "Copyright"

At step 3 the file is removed from the list, and at step 4 the files is readded, but since the file was removed from the model, we lost the collapsed information so we don't know that is was collapsed before.

If this is the only way to reproduce that bug, then it is a bug in master too and I don't think that is the same bug as #9363. So we should open a new issue for it and I can fix it in another PR, so that we can merge this one earlier.

[redmunds]

@TomMalbran I'm editing a simple HTML page. I do a Find in Files on just that page for "body". 2 instances found, and I collapse only node. Then I edit a tag other than body tag, Ctrl-S to save, and results are expanded.

UPDATED: I'm seeing the same results in master, so I guess this is not related to this PR

[TomMalbran]

@redmunds I see. I was able to reproduce it now. But it is also a bug in master and it was not created in this PR. I think that the file is searched again and all the results are replaced, removing the state of isCollapsed. Do you think that we still need to fix it here?

[redmunds]

@TomMalbran No, that doesn't need to get fixed here.

[redmunds]

Thanks. Merging.

[TomMalbran]

@redmunds I tried to track the last issue you mentioned and I finally found it. When you save the file, the file system triggers a change event, and it deletes the entry from the search model. After that the file is searched again and readded, but the collapsed information was lost.

I am thinking that maybe we should keep the collapsed state in a different object.