873298 - Change "Filter scripts" string to "Search scripts"

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Comment 1

[Heather Arthur [:harth]]

My bad, submitted form too soon ^

It would never have occurred to me that you could search within scripts using the "Filter scripts" input. When I hear filter I think of lists being pared down, so it sounds like it will filter through script names.

In reality, it does much more than that, and I think "Search scripts" would represent that much better.

Comment 2

[Rob Campbell [:rc] (:robcee)]

Agreed. Want to mentor this? Should be a good first bug.

Comment 3

[Sumedh Shekhar]

I would like to work on it. 

I believe editing the following should do the job. 
http://mxr.mozilla.org/mozilla-central/search?string=%22filter+scripts%22&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 4

[Heather Arthur [:harth]]

Thanks Sumedh. That's the place to edit. Whenever we change a string we have to change the string name as well, even "searchFile2" would work.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

But do note that filtering scripts is the default behavior of the input box, without any additional operator prefix. If we make the default placeholder text more generic, then we probably should add an entry about that to the tooltip that appears when focusing the input.

Comment 6

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #5)

You have my vote.

I'd also prefer if we take this opportunity and prefer the slightly more accurate "Search sources" instead of "Search scripts".

Comment 7

[Sumedh Shekhar]

(In reply to Panos Astithas [:past] from comment #5)
> But do note that filtering scripts is the default behavior of the input box,
> without any additional operator prefix. If we make the default placeholder
> text more generic, then we probably should add an entry about that to the
> tooltip that appears when focusing the input.

What kind of entry do you suggest?

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

How about using the existing placeholder?

Comment 9

[Sumedh Shekhar]

(In reply to Panos Astithas [:past] from comment #8)
> How about using the existing placeholder?

If i understand you correctly, you suggest adding "Filter scripts(Ctrl+P)" to the already present list in placeholder?

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Right, probably with the text "none" instead of a button on the left column.

Comment 11

[Sunny [:darkowlzz]]

Sumedha came to irc asking about this bug, and I suggested these:

1/ In [1], Change |emptyFilterText| to |emptySearchText| with value `Search scripts`. Also add another literal, something like |FilterText| with value `Filter scripts`.

2/ In [2], replace |emptyFilterText| with |emptySearchText| as we have changed the literal in 1/. This would take care of changing what appears in the Search box when it's empty.

3/ To add `Filter Script` to the searchbox help panel, add a xul |hbox| in [3]. As it doesn't has any operator key associated with it, you don't need a button here, but from Comment 10, you need a text "none" which could be a |label| with value "none". Another |label| with id, something like `filter-operator-label`, would be good enough.
For the |label| value, you need to add a new property in [4], something like |this._filterOperatorLabel|, and query document like this
   
  this._filterOperatorLabel = document.getElementById("filter-operator-label");

Once you have the element, set it's value like as in [5]. Set the 1st parameter of |getFormatStr| as |FilterText|, from 1/ above, and 2nd parameter as |this._fileSearchKey|. |this._fileSearchKey| has the key combination Ctrl+P, and that is what is required here. :)

(In reply to Heather Arthur [:harth] from comment #4)
> Thanks Sumedh. That's the place to edit. Whenever we change a string we have
> to change the string name as well, even "searchFile2" would work.
I think that is not required. `searchFile` is being used in the Right-click contextMenu of the debugger too, which is not what we need. For this bug, you need to change in [1].

Hope, the it helps and please correct me if I went wrong somewhere.

Thanks!

[1] http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/debugger.properties#99
[2] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-toolbar.js#792
[3] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger.xul#309
[4] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-toolbar.js#719
[5] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-toolbar.js#749

Comment 12

[Sumedh Shekhar]

Attachment: Replaced "Filter scripts" by "Search sources". Added Filter scripts to the popup

Added "Filter scripts" to the popup but it looks a little dirty this way.

Comment 13

[Victor Porof [:vporof][:vp]]

Please also ask me for review once this patch is ready. Thank you!

Comment 14

[Sunny [:darkowlzz]]

Comment on attachment 765457 [details] [diff] [review]
Replaced "Filter scripts" by "Search sources". Added Filter scripts to the popup

Review of attachment 765457 [details] [diff] [review]:
-----------------------------------------------------------------

I compiled it and I see it is working :)
There are some nits which I would like to point out.

::: browser/devtools/debugger/debugger-toolbar.js
@@ +725,5 @@
>      this._lineOperatorButton = document.getElementById("line-operator-button");
>      this._lineOperatorLabel = document.getElementById("line-operator-label");
>      this._variableOperatorButton = document.getElementById("variable-operator-button");
>      this._variableOperatorLabel = document.getElementById("variable-operator-label");
> +    this._filterOperatorLabel = document.getElementById("filter-operator-label");

I think you should move this up, just after |this._searchboxHelpPanel|.

@@ +757,5 @@
>        L10N.getFormatStr("searchPanelLine", this._lineSearchKey));
>      this._variableOperatorLabel.setAttribute("value",
>        L10N.getFormatStr("searchPanelVariable", this._variableSearchKey));
> +    this._filterOperatorLabel.setAttribute("value",
> +	  L10N.getFormatStr("filterText", this._fileSearchKey));

Nit: Do not mix tabs and spaces.
Move this up, just before |this._globalOperatorLabel|.

::: browser/devtools/debugger/debugger.xul
@@ +340,5 @@
>                  command="variableSearchCommand"/>
>          <label id="variable-operator-label"
>                 class="plain searchbox-panel-operator-label"/>
>        </hbox>
> +      <hbox align="center">

I think it would be better to move this to the top, as it's the default behavior. Make it the first |hbox| in this |vbox|.

@@ +341,5 @@
>          <label id="variable-operator-label"
>                 class="plain searchbox-panel-operator-label"/>
>        </hbox>
> +      <hbox align="center">
> +		 <label value="(None)"/>

Correct the indentation. Do not mix tabs with spaces. Move it to the left, align with the following |label|.
I am not sure about the |value|, harth would be able to tell you about it's proper value.

::: browser/locales/en-US/chrome/browser/devtools/debugger.properties
@@ +112,5 @@
>  searchPanelGlobal=Search in all files (%S)
>  
> +# LOCALIZATION NOTE (filterText): This is the text that appears in the
> +# filter panel popup for the script search operation.
> +filterText=Filter Scripts (%S) 

I am not sure if it really matters, but looking at other literals values, I think `Scripts` should be `scripts`. :)
Also move this up, just before |searchPanelGlobal|.
Remove the whitespace at the end of this line.

Comment 15

[Sunny [:darkowlzz]]

I missed it.

::: browser/devtools/debugger/debugger.xul
> @@ +341,5 @@
> >          <label id="variable-operator-label"
> >                 class="plain searchbox-panel-operator-label"/>
> >        </hbox>
> > +      <hbox align="center">
> > +		 <label value="(None)"/>

This works, but I think if you use |label| in this way, it's value should be defined using some script as is done for other labels. You could follow other labels or have a look at [1].

[1] http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/web/viewer.html?force=1#63

Comment 16

[Heather Arthur [:harth]]

Comment on attachment 765457 [details] [diff] [review]
Replaced "Filter scripts" by "Search sources". Added Filter scripts to the popup

Review of attachment 765457 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Sumedh and Sunny. I have a UX issue:

::: browser/devtools/debugger/debugger.xul
@@ +341,5 @@
>          <label id="variable-operator-label"
>                 class="plain searchbox-panel-operator-label"/>
>        </hbox>
> +      <hbox align="center">
> +		 <label value="(None)"/>

You would have to localize that "(None)".

But, the (None) looks pretty weird to me. Maybe we could consider having "Filter Scripts (Cmd-P)" at the top of the popup, followed by the "Operators: ...."

What do others think about that?

Comment 17

[Sumedh Shekhar]

(In reply to Heather Arthur [:harth] from comment #16)
> Comment on attachment 765457 [details] [diff] [review]
> Replaced "Filter scripts" by "Search sources". Added Filter scripts to the
> popup
> 
> Review of attachment 765457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks Sumedh and Sunny. I have a UX issue:
> 
> ::: browser/devtools/debugger/debugger.xul
> @@ +341,5 @@
> >          <label id="variable-operator-label"
> >                 class="plain searchbox-panel-operator-label"/>
> >        </hbox>
> > +      <hbox align="center">
> > +		 <label value="(None)"/>
> 
> You would have to localize that "(None)".
> 
> But, the (None) looks pretty weird to me. Maybe we could consider having
> "Filter Scripts (Cmd-P)" at the top of the popup, followed by the
> "Operators: ...."
> 
> What do others think about that?

Thanks for the review. I too agree it looks strange. I think even adding it the way you mentioned too would'nt be too nice to look at. I will anyway, upload a patch with all the corrections and "Filter scripts (Ctrl+P)" above operators very soon and we could decide then.

Comment 18

[Sumedh Shekhar]

Attachment: Corrected the space-tab mix ups, Moved Filter scripts (Ctrl+P) to the top. Removed "(none)"

Removing "(none)" and moving Filter scripts above operators looks better I think.

Comment 19

[Heather Arthur [:harth]]

Comment on attachment 769387 [details] [diff] [review]
Corrected the space-tab mix ups, Moved Filter scripts (Ctrl+P) to the top. Removed "(none)"

Flagging Victor for review too.

Comment 20

[Heather Arthur [:harth]]

Comment on attachment 769387 [details] [diff] [review]
Corrected the space-tab mix ups, Moved Filter scripts (Ctrl+P) to the top. Removed "(none)"

Review of attachment 769387 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks good to me, and the separate "Filter Scripts" looks better than the (None), I think.

I have a few small things that could just be fixed before checking in. But, would like to see this UI issue addressed first: 1) more space between "Filter Scripts" and "Operators" 2) "Operators" -> "Operators:" 3) "Filter Scripts" vertically flush with "Operators", if possible (this one isn't so much of a big deal). These would involve editing the CSS in browser/themes/<os>/devtools/debugger.css.

Thanks again for the patch!

::: browser/devtools/debugger/debugger-toolbar.js
@@ +726,5 @@
>      this._lineOperatorButton = document.getElementById("line-operator-button");
>      this._lineOperatorLabel = document.getElementById("line-operator-label");
>      this._variableOperatorButton = document.getElementById("variable-operator-button");
>      this._variableOperatorLabel = document.getElementById("variable-operator-label");
> +    

Looks like you added some trailing whitespace here.

::: browser/devtools/debugger/debugger.xul
@@ +303,5 @@
>           type="arrow"
>           noautofocus="true"
>           position="before_start">
>      <vbox>
> +      <hbox align="center">

don't need the align="center"

@@ +305,5 @@
>           position="before_start">
>      <vbox>
> +      <hbox align="center">
> +        <label id="filter-operator-label"
> +                class="plain searchbox-panel-operator-label"/>

Get rid of "searchbox-panel-operator-label" class, as it's not really one of the operators anymore, and looks like it adds some unnecessary style. Align the "class" attribute with the "id" attribute.

@@ +311,2 @@
>        <label id="searchbox-panel-description"
>               value="&debuggerUI.searchPanelTitle;"/>

This is the "Operators" label, which isn't so much of a title anymore.

Comment 21

[Victor Porof [:vporof][:vp]]

Comment on attachment 769387 [details] [diff] [review]
Corrected the space-tab mix ups, Moved Filter scripts (Ctrl+P) to the top. Removed "(none)"

Review of attachment 769387 [details] [diff] [review]:
-----------------------------------------------------------------

What Heather said!

Comment 22

[Heather Arthur [:harth]]

Sumedh, you have time to finish this up?

Comment 23

[Sumedh Shekhar]

Sorry, I was out for a while. Would finish this pretty soon.

Comment 24

[Sumedh Shekhar]

Attachment: Minor adjustments suggested by Heather

Have'nt checked it on osx and windows as i only have linux.

Comment 25

[Heather Arthur [:harth]]

Comment on attachment 777977 [details] [diff] [review]
Minor adjustments suggested by Heather

Review of attachment 777977 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Sumedh! I didn't mean to rush you, I was just making sure.

This looks good on OS X too. I have a few mentions. I'll change them myself before checking in, but keep them in mind for next time.

::: browser/devtools/debugger/debugger-toolbar.js
@@ +715,5 @@
>      dumpn("Initializing the FilterView");
>  
>      this._searchbox = document.getElementById("searchbox");
>      this._searchboxHelpPanel = document.getElementById("searchbox-help-panel");
> +    this._filterOperatorLabel = document.getElementById("filter-operator-label");

The filter isn't technically an operator like the others, so probably shouldn't carry that around the variable names.

@@ +726,5 @@
>      this._lineOperatorButton = document.getElementById("line-operator-button");
>      this._lineOperatorLabel = document.getElementById("line-operator-label");
>      this._variableOperatorButton = document.getElementById("variable-operator-button");
>      this._variableOperatorLabel = document.getElementById("variable-operator-label");
> +    

trailing whitespace.

::: browser/devtools/debugger/debugger.xul
@@ +310,1 @@
>        <label id="searchbox-panel-description"

"searchbox-panel-description" -> "searchbox-panel-operators" or something.

::: browser/locales/en-US/chrome/browser/devtools/debugger.dtd
@@ +75,2 @@
>    -  appears in the filter panel popup as a description. -->
> +<!ENTITY debuggerUI.searchPanelOperations    "Operators:">

"Operations" -> "Operators"

::: browser/locales/en-US/chrome/browser/devtools/debugger.properties
@@ +108,5 @@
>  emptyVariablesFilterText=Filter variables
>  
> +# LOCALIZATION NOTE (filterText): This is the text that appears in the
> +# filter panel popup for the script search operation.
> +filterText=Filter scripts (%S)

"filterText" -> "searchPanelFilter". Goes along better with the other names below like "searchPanelGlobal".

::: browser/themes/linux/devtools/debugger.css
@@ +104,5 @@
>    -moz-margin-start: 1px;
>  }
>  
> +#filter-operator-label {
> +  -moz-margin-start: 2px;

thanks for doing -moz-margin-start!

Comment 26

[Heather Arthur [:harth]]

http://hg.mozilla.org/integration/fx-team/rev/6e8ad2f613ff

Thanks again!

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/6e8ad2f613ff