Closed Bug 1172500 Opened 4 years ago Closed 4 years ago

Keyboard shortcut to toggle the bottom-docked host minimize mode

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 867838 brings the ability to minimize the bottom-docked toolbox host.
We should add a keyboard shortcut for this to be more useful.
Depends on: 867838
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Bug 1172500 - Adds a keyboard shortcut to minimize the toolbox; r=jryans
Attachment #8617296 - Flags: review?(jryans)
On top of the fix, the patch contains some minor eslint-related cleanups.
Comment on attachment 8617296 [details]
MozReview Request: Bug 1172500 - Adds a keyboard shortcut to minimize the toolbox; r=jryans

https://reviewboard.mozilla.org/r/10607/#review9309

Ctrl-Shift-M is the shortcut for Responsive Design[1] on Windows / Linux.

I'm afraid you'll have to roll your shortcut dice again.

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#300-306

::: browser/devtools/framework/toolbox.js:35
(Diff revision 1)
> -loader.lazyGetter(this, "Hosts", () => require("devtools/framework/toolbox-hosts").Hosts);
> +loader.lazyGetter(this, "Hosts", () => {

Nit: You could use lazyRequireGetter:

loader.lazyRequireGetter(this, "Hosts", "devtools/framework/toolbox-hosts", true);

Same for more lines below.  I know you are just cleaning up for the linter so... up to you! :)

::: browser/devtools/framework/toolbox.js:758
(Diff revision 1)
> +           (osString == "Darwin" ? "Cmd+Shift+" : "Ctrl+Shift+") +

Is this really the only place we do this shortcut labelling?  Maybe it should be a shared utility function?
Attachment #8617296 - Flags: review?(jryans)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> Comment on attachment 8617296 [details]
> MozReview Request: Bug 1172500 - Adds a keyboard shortcut to minimize the
> toolbox; r=jryans
> 
> https://reviewboard.mozilla.org/r/10607/#review9309
> 
> Ctrl-Shift-M is the shortcut for Responsive Design[1] on Windows / Linux.
> 
> I'm afraid you'll have to roll your shortcut dice again.
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> sets.inc#300-306
Damn ... What's the logic behind some of our shortcuts that use accel+shift on mac and accel+alt on PC? Why not always the same?
> ::: browser/devtools/framework/toolbox.js:35
> (Diff revision 1)
> > -loader.lazyGetter(this, "Hosts", () => require("devtools/framework/toolbox-hosts").Hosts);
> > +loader.lazyGetter(this, "Hosts", () => {
> 
> Nit: You could use lazyRequireGetter:
> 
> loader.lazyRequireGetter(this, "Hosts", "devtools/framework/toolbox-hosts",
> true);
> 
> Same for more lines below.  I know you are just cleaning up for the linter
> so... up to you! :)
Cleaned this up, thanks for the tip, I had never really looked into how Loader.jsm worked, that was a good opportunity for doing this.
> ::: browser/devtools/framework/toolbox.js:758
> (Diff revision 1)
> > +           (osString == "Darwin" ? "Cmd+Shift+" : "Ctrl+Shift+") +
> 
> Is this really the only place we do this shortcut labelling?  Maybe it
> should be a shared utility function?
When I wrote the patch, yeah it was (or close), now a change to definition.js has landed that introduced a whole bunch of new shortcut labelling, so we could do better. I'll file a bug for this.
Attachment #8617296 - Flags: review?(jryans)
Comment on attachment 8617296 [details]
MozReview Request: Bug 1172500 - Adds a keyboard shortcut to minimize the toolbox; r=jryans

Bug 1172500 - Adds a keyboard shortcut to minimize the toolbox; r=jryans
https://reviewboard.mozilla.org/r/10607/#review9371

> Is this really the only place we do this shortcut labelling?  Maybe it should be a shared utility function?

See bug 1173291
(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #4)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> > Comment on attachment 8617296 [details]
> > MozReview Request: Bug 1172500 - Adds a keyboard shortcut to minimize the
> > toolbox; r=jryans
> > 
> > https://reviewboard.mozilla.org/r/10607/#review9309
> > 
> > Ctrl-Shift-M is the shortcut for Responsive Design[1] on Windows / Linux.
> > 
> > I'm afraid you'll have to roll your shortcut dice again.
> > 
> > [1]:
> > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> > sets.inc#300-306
> Damn ... What's the logic behind some of our shortcuts that use accel+shift
> on mac and accel+alt on PC? Why not always the same?

No idea why it's like that really... :/
Comment on attachment 8617296 [details]
MozReview Request: Bug 1172500 - Adds a keyboard shortcut to minimize the toolbox; r=jryans

https://reviewboard.mozilla.org/r/10607/#review9405

This shortcut appears available on all OSes.  Can you post a try run so builds can be checked to be sure?

::: browser/devtools/framework/toolbox.js:37
(Diff revisions 1 - 2)
> +               "chrome://browser/locale/devtools/toolbox.properties");

Nit: This is kind of unusual wrapping...  Maybe put this path in a const var and use it here instead?
Attachment #8617296 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> This shortcut appears available on all OSes.  Can you post a try run so
> builds can be checked to be sure?
Yes sure, I actually already ran 2 try builds, but didn't post them because some tests were failing on linux. I'm currently investigating and will post a fix and new try build very shortly.
I went nuts this morning trying to figure out a shortcut to use.
Turns out accel+shift+- didn't work on linux. I tested a whole lot of them only to find that a lot of them just don't work even if they're not taken already.
I settled for accel+shift+U which works everywhere and isn't taken.
It seems to work nicely: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c8f4e965055
(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #10)
> I went nuts this morning trying to figure out a shortcut to use.
> Turns out accel+shift+- didn't work on linux. I tested a whole lot of them
> only to find that a lot of them just don't work even if they're not taken
> already.
> I settled for accel+shift+U which works everywhere and isn't taken.
> It seems to work nicely:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c8f4e965055

Okay, if you've tested all 3 OSes, then it sounds good to me!
Keywords: checkin-needed
Keywords: checkin-needed
Last changes as discussed earlier, and rebased on top of latest fx-team.
Try is green.
Ready to be checked-in.
Attachment #8617296 - Attachment is obsolete: true
Attachment #8621434 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/15f05333624f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.