[replace-across-files] Replace in Files #7809
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -735,23 +735,31 @@ define(function (require, exports, module) { | |
| * Differs from plain FileUtils.readAsText() in two ways: (a) line endings are still normalized | ||
| * as in Document.getText(); (b) unsaved changes are returned if there are any. | ||
| * | ||
| * @param {!File} file The file to get the text for. | ||
| * @param {boolean=} checkLineEndings Whether to return line ending information. Default false (slightly more efficient). | ||
| * @return {$.Promise} | ||
| * A promise that is resolved with three parameters: | ||
| * contents - string: the document's text | ||
| * timestamp - Date: the last time the document was changed on disk (might not be the same as the last time it was changed in memory) | ||
| * lineEndings - string: the original line endings of the file, one of the FileUtils.LINE_ENDINGS_* constants; | ||
| * will be null if checkLineEndings was false. | ||
| * or rejected with a filesystem error. | ||
|
There was a problem hiding this comment. Currently, this block of info about There was a problem hiding this comment. Pretty sure we have many methods where Does moving the "A promise that..." part onto the same line as There was a problem hiding this comment. Has nothing to do with I just noticed that the entire FYI, by installing apify and then brackets-apify extension, you can quickly render a temporary version of API Docs to see what your annotations look like before submitting PR. There was a problem hiding this comment. I just remembered a bug recently fixed by @jbalsas where There was a problem hiding this comment. OK - so sounds like I don't need to change this then? There was a problem hiding this comment. @njx I opened an issue: jbalsas/apify#69 |
||
| */ | ||
| function getDocumentText(file, checkLineEndings) { | ||
| var result = new $.Deferred(), | ||
| doc = getOpenDocumentForPath(file.fullPath); | ||
| if (doc) { | ||
| result.resolve(doc.getText(), doc.diskTimestamp, checkLineEndings ? doc._lineEndings : null); | ||
| } else { | ||
| file.read(function (err, contents, stat) { | ||
| if (err) { | ||
| result.reject(err); | ||
| } else { | ||
| // Normalize line endings the same way Document would, but don't actually | ||
| // new up a Document (which entails a bunch of object churn). | ||
| var originalLineEndings = checkLineEndings ? FileUtils.sniffLineEndings(contents) : null; | ||
|
There was a problem hiding this comment. This stuff seems use-case-specific enough that I wonder if the Replace code should just call There was a problem hiding this comment. We want to get the document text if it's already open, though. I don't want to duplicate all the functionality in this method. Plus, I imagine that we could easily have other batch-processing functionality in the future that wants to do something similar across files, where it wants the document text if it's in memory but on-disk text with line ending information if it's on disk. |
||
| contents = DocumentModule.Document.normalizeText(contents); | ||
| result.resolve(contents, stat.mtime, originalLineEndings); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,24 +1,31 @@ | ||
| <div class="find-input-group"> | ||
|
There was a problem hiding this comment. The DOM structure here changed slightly since the original unification so that I could make the scope controls There was a problem hiding this comment. I don't quite like how the Replace in Files bar has usually 2 lines, but the fix I am thinking requires the multiple exclusions select. My idea is to make the Exclusions select take the exact same space as the navigation, since Find/Replace have navigation only and Find/Replace in Files have only exclusions. The other thing to fix is the scope that can take a lots of space. I was thinking of just placing the current folder / in project text inside There was a problem hiding this comment. @TomMalbran We'll have Exclusions on Find/Replace soon. I agree that the controls should all be on 1 line if there is enough horizontal width for it. There was a problem hiding this comment. It is already in master, so we can apply this fix by making the exclusions sets select have always a fixed width that is the exact same width as the prev and next buttons combined. And if we then move the scope to another place, the Replace in Files bar can have the exact same width as the Replace bar which does look nice. There was a problem hiding this comment. @TomMalbran It seems like we couldn't really shrink down the exclusion-picker dropdown to the tiny width that the next+prev buttons currently occupy, though. (And it seems like you'd have to fit the scope label in that amount of space too, which would be basically impossible even if you change it to only show the last folder name). And if we didn't shrink that set of UI way down, then the problem remains that there's not enough width to reliably fit it, thus it needs to remain wrappable. There was a problem hiding this comment. I had tried that and it looks funny too. Basically, there isn't really a good solution that doesn't leave an awkward set of whitespace somewhere (without doing something more radical). For now, if we're just going to keep all the controls in the bar, it seemed better to just have the fields line up vertically. It would be nice to find a more compact solution eventually. There was a problem hiding this comment. It did look good to me with that change, and I didn't got any space. But I only tried the solution in the dev tools and I changed the replace group to be inline-block so that it was placed after the exceptions button. I also changed the exceptions text + edit button with the new button, since this branch doesn't have the new exclusions sets. I don't like how it looks right now. Even just Find in Files looks kind of bad with the scope in between the find group and the exclusions set. Maybe the scope can later become a select button, so that you can change the scope after opening the find bar. There was a problem hiding this comment. Oh, I didn't try just having the scope in the second line. But having the exclusions in the top line with everything else seems like it might get crowded too. I'll try to fiddle with it a bit if I get time, but if not, maybe you can put up a PR with your suggestion for @larz0 to review after this lands. There was a problem hiding this comment. Sure. Notice that I am using the new exclusions button, since is a lot cleaner for the UI and I gave it a fixed width, since right now the width changes depending on the name of the exclusion set and it can potentially take a lot of space. There was a problem hiding this comment. Fwiw, I also like the idea of putting the find+replace on one line and the scope+exclusions on the second line... I think it would read a little more cleanly than the layout today. Not urgent though -- something we could tweak after this all lands in master, or even in a later sprint... |
||
| <div id="find-group"><!-- | ||
| --><div class="search-input-container"><input type="text" id="find-what" placeholder="{{queryPlaceholder}}" value="{{initialQuery}}" /><div class="error"></div><span id="find-counter"></span></div><!-- | ||
| --><button id="find-case-sensitive" class="btn no-focus" tabindex="-1" title="{{Strings.BUTTON_CASESENSITIVE_HINT}}"><div class="button-icon"></div></button><!-- | ||
| --><button id="find-regexp" class="btn no-focus" tabindex="-1" title="{{Strings.BUTTON_REGEXP_HINT}}"><div class="button-icon"></div></button><!-- | ||
| {{#navigator}} | ||
| --><div class="navigator"><!-- | ||
| --><button id="find-prev" class="btn no-focus" tabindex="-1" title="{{Strings.BUTTON_PREV_HINT}}">{{Strings.BUTTON_PREV}}</button><!-- | ||
| --><button id="find-next" class="btn no-focus" tabindex="-1" title="{{Strings.BUTTON_NEXT_HINT}}">{{Strings.BUTTON_NEXT}}</button><!-- | ||
| --></div><!-- | ||
| {{/navigator}} | ||
| --></div> | ||
|
|
||
| {{#replace}} | ||
| <div id="replace-group" {{#scope}}class="has-scope"{{/scope}}><!-- | ||
| --><input type="text" id="replace-with" placeholder="{{Strings.REPLACE_PLACEHOLDER}}"/><!-- | ||
| {{^replaceAllOnly}} | ||
| --><button id="replace-yes" class="btn no-focus" tabindex="-1">{{Strings.BUTTON_REPLACE}}</button><!-- | ||
| {{/replaceAllOnly}} | ||
| --><button id="replace-all" class="btn no-focus {{#replaceAllOnly}}solo{{/replaceAllOnly}}" tabindex="-1">{{replaceAllLabel}}</button></div> | ||
|
There was a problem hiding this comment.
|
||
| {{/replace}} | ||
| </div> | ||
|
|
||
| {{#scope}} | ||
| <div class="scope-group"> | ||
| <div class="no-results-message">{{Strings.FIND_NO_RESULTS}}</div> | ||
| <div class="message"> | ||
| <span id="searchlabel">{{{scopeLabel}}}</span> | ||
| </div> | ||
| </div> | ||
| {{/scope}} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was to make it so we get a
documentChangeevent from the global Document module consistently on reverts, even if there is no masterEditor (this is used by FindInFiles to know when a document has changed). However, we don't want to get such an event when the Document is first created, because that's not really a "change".