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

[ON HOLD] Scope $() searches to the current ModalBar#7512

Closed
njx wants to merge 4 commits into
masterfrom
nj/issue-6617
Closed

[ON HOLD] Scope $() searches to the current ModalBar#7512
njx wants to merge 4 commits into
masterfrom
nj/issue-6617

Conversation

@njx
Copy link
Copy Markdown

@njx njx commented Apr 15, 2014

For #6617. The problem there was that when the new ModalBar opens, the old ModalBar isn't done closing yet, and so $() searches for DOM elements were finding items in the old ModalBar rather than the new one. This scopes them to the new ModalBar.

@njx
Copy link
Copy Markdown
Author

njx commented Apr 15, 2014

@peterflynn Care to look at this one? It's pretty straightforward. If not, please feel free to reassign.

Comment thread src/search/FindReplace.js Outdated
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This was the only interesting change :) - without this, the empty string would end up getting sent to the replace all panel.

@njx
Copy link
Copy Markdown
Author

njx commented Apr 15, 2014

Needs more work, not ready for review yet.

@njx
Copy link
Copy Markdown
Author

njx commented Apr 15, 2014

Hmmm, a lot more surgery is necessary for Find in Files, because there, the modalBar lives in the FindInFilesDialog instance, but a lot of the logic that queries for UI elements is in module functions that aren't in the dialog instance. It would probably be better to make that model logic non-UI-dependent, but the easier course in the short term would be to just move it all into the FindInFilesDialog.

@njx
Copy link
Copy Markdown
Author

njx commented Apr 15, 2014

Actually, it's not that much stuff - just _getQueryRegexp(), which could probably be moved into the dialog, and some logic in _showSearchResults(). The rest of it might be independent enough, since those other functions already depend on a module global that caches the current query.

@TomMalbran
Copy link
Copy Markdown
Contributor

We could just remove the FindInFilesDialog class. It doesn't make much sense to have a class for an object that should have only 1 instance. Find and Replace don't have a class for their dialog, so maybe Find in Files shouldn't either.

@njx
Copy link
Copy Markdown
Author

njx commented Apr 15, 2014

True, though I tend to like things to be in classes :) But yeah, if it's a singleton anyway, and looking at the code it's not doing much anyway, so maybe it's not necessary. I probably introduced that class myself awhile ago, but I don't recall. @peterflynn what do you think - should we just flatten the dialog class into the module?

@njx
Copy link
Copy Markdown
Author

njx commented Apr 15, 2014

Hmmm, it looks like we might want to do some refactoring for find in files in #7445 anyway. So maybe I'll hold off on fixing up Find in Files and just restrict this to Find.

@njx
Copy link
Copy Markdown
Author

njx commented Apr 15, 2014

OK, I've changed all the ids to classes (in both Find and Find in Files, but I didn't make the overall fix for Find in Files yet).

@njx
Copy link
Copy Markdown
Author

njx commented Apr 25, 2014

Hmmm...even though the only thing I changed in Find in Files was the ids to classes (and didn't attempt to actually fix the bug there), it seems to be broken in this branch (closing it and then trying to reopen it again doesn't work at all). So this is still not ready for review yet.

@njx njx changed the title Scope $() searches to the current ModalBar [ON HOLD] Scope $() searches to the current ModalBar Apr 25, 2014
@njx
Copy link
Copy Markdown
Author

njx commented Apr 25, 2014

Actually, it looks like that bug is in master too. Foo. So we should really fix that at the same time.

Further, it looks like we have some code that synchronizes between the Find in Files version of the find dialog and the Find/Replace version (for maintaining the state of the case/regexp buttons), which now needs to know which modal bar to look at.

At this point I think I should kill this PR, and just start working on Replace Across Multiple Files, as part of which I think we'll want to refactor the various find UIs into a single UI class anyway. Once I do that, then the various cases of this bug can all be fixed together.

Closing.

@njx njx closed this Apr 25, 2014
@njx
Copy link
Copy Markdown
Author

njx commented Apr 25, 2014

Please don't delete the branch for this PR yet.

@njx
Copy link
Copy Markdown
Author

njx commented May 13, 2014

I'm going to go ahead and delete this branch now.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants