Change "Filter scripts" string to "Search scripts"

RESOLVED FIXED in Firefox 25

Status

DevTools
Debugger
P3
normal
RESOLVED FIXED
5 years ago
10 days ago

People

(Reporter: harth, Assigned: Sumedh Shekhar)

Tracking

unspecified
Firefox 25

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good-first-bugs][lang=xul][mentor=harth])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
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.
Summary: Change "Filter Scripts → Change "Filter scripts" string to "Search scripts"
Agreed. Want to mentor this? Should be a good first bug.
Whiteboard: [good-first-bugs][lang=xul][mentor=harth]
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 3

5 years ago
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
(Reporter)

Comment 4

5 years ago
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.
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.
(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".
(Assignee)

Comment 7

5 years ago
(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?
How about using the existing placeholder?
(Assignee)

Comment 9

5 years ago
(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?
Right, probably with the text "none" instead of a button on the left column.

Comment 11

5 years ago
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
Priority: -- → P3
(Assignee)

Comment 12

5 years ago
Created attachment 765457 [details] [diff] [review]
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.
Attachment #765457 - Flags: review?(fayearthur)
(Assignee)

Updated

5 years ago
Attachment #765457 - Flags: feedback?(indiasuny000)
Please also ask me for review once this patch is ready. Thank you!

Comment 14

5 years ago
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.
Attachment #765457 - Flags: feedback?(indiasuny000) → feedback+

Comment 15

5 years ago
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
(Reporter)

Comment 16

5 years ago
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?
(Assignee)

Comment 17

5 years ago
(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.
(Assignee)

Comment 18

5 years ago
Created attachment 769387 [details] [diff] [review]
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.
Attachment #769387 - Flags: review?(fayearthur)
Attachment #769387 - Flags: feedback?(indiasuny000)
(Reporter)

Updated

5 years ago
Attachment #765457 - Attachment is obsolete: true
Attachment #765457 - Flags: review?(fayearthur)
(Reporter)

Comment 19

5 years ago
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.
Attachment #769387 - Flags: review?(vporof)
(Reporter)

Comment 20

5 years ago
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 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!
Attachment #769387 - Flags: review?(vporof) → feedback+

Updated

5 years ago
Attachment #769387 - Flags: feedback?(indiasuny000) → feedback+
(Reporter)

Comment 22

5 years ago
Sumedh, you have time to finish this up?
(Assignee)

Comment 23

5 years ago
Sorry, I was out for a while. Would finish this pretty soon.
(Assignee)

Comment 24

5 years ago
Created attachment 777977 [details] [diff] [review]
Minor adjustments suggested by Heather

Have'nt checked it on osx and windows as i only have linux.
Attachment #769387 - Attachment is obsolete: true
Attachment #769387 - Flags: review?(fayearthur)
Attachment #777977 - Flags: review?(fayearthur)
(Reporter)

Comment 25

5 years ago
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!
Attachment #777977 - Flags: review?(fayearthur) → review+
(Reporter)

Comment 26

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

Thanks again!
Whiteboard: [good-first-bugs][lang=xul][mentor=harth] → [good-first-bugs][lang=xul][mentor=harth][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6e8ad2f613ff
Assignee: nobody → sumedhhere
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good-first-bugs][lang=xul][mentor=harth][fixed-in-fx-team] → [good-first-bugs][lang=xul][mentor=harth]
Target Milestone: --- → Firefox 25

Updated

10 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.