1669710 - Rename (preview, nonPreview) searchMode to (preview, confirmed)

Status: RESOLVED FIXED

(Firefox :: Address Bar, task, P3)

Description

[Marco Bonardo [:mak]]

I think using temporary and permanent for searchMode, instead of preview and nonPreview, would make the code more understandable.

Comment 1

[Drew Willcoxon :adw]

I do think there's room for improvement with nonPreview at least. temporary and permanent make sense from one point of view, but they could also be misleading since permanent might imply that search mode could never be exited once entered. Maybe I'm overthinking it though. I would be OK with them.

Personally I do like preview since it captures the idea of not yet entering search mode but showing you what it looks like. The problem is that it doesn't have a clear opposite in this context, at least not one I can think of. I used the terms "full search mode" and "actual search mode" while reviewing the patch where Harry added preview mode, so maybe full?

Comment 2

[Marco Bonardo [:mak]]

I was also thinking confirmed or settled, but yes when speaking we always referred to it as full, that why it's not exactly representing the user action, it's easy to grasp.

Comment 3

[Harry Twyford]

I agree with Drew that preview is an accurate description. I also like confirm(ed) since we can use it both as a verb and as an adjective. That makes the most sense with the way we've been writing comments. For example, we have this comment:

// Search modes are per browser and are stored in this map.  For a 
// browser, search mode can be in preview mode, non-preview mode, or both.
// Typically, search mode is entered in preview mode with a particular
// source and is promoted to non-preview mode with the same source once a
// query starts.  It's also possible for a non-preview mode to be replaced
// with a preview mode with a different source, and in those cases, we need
// to be able to restore the non-preview mode when preview mode is exited.
// In addition, only non-preview mode should be restored across sessions.
// We therefore need to keep track of both the current non-preview and
// preview modes, per browser.

Using preview and confirmed flows well:

// Search modes are per browser and are stored in this map.  For a 
// browser, search mode can be in preview mode, confirmed, or both.
// Typically, search mode is entered in preview mode with a particular
// source and is confirmed with the same source once a
// query starts.  It's also possible for a confirmed search mode to be replaced
// with a preview mode with a different source, and in those cases, we need
// to re-confirm search mode when preview mode is exited.
// In addition, only confirmed search modes should be restored across sessions.
// We therefore need to keep track of both the current confirmed and
// preview modes, per browser.

It also works if we instead replace all the references to non-preview mode with full mode, but that's a bit clunky IMO. Thoughts?

Comment 4

[Drew Willcoxon :adw]

Sounds good! [Edit: confirmed, to be clear]

Comment 5

[Marco Bonardo [:mak]]

ship it!

Comment 6

[Harry Twyford]

Attachment: Bug 1669710 - Rename (preview, nonPreview) searchMode to (preview, confirmed). r?adw!

Comment 7

[Harry Twyford]

There are a couple of small code changes in this patch, so here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b210a2b465a4454e3658c89751fd5a510d0c3e2

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45d432067093
Rename (preview, nonPreview) searchMode to (preview, confirmed). r=adw

Comment 9

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/45d432067093