Closed Bug 656770 Opened 14 years ago Closed 14 years ago

Display keyboard shortcuts

Categories

(Tree Management Graveyard :: TBPL, defect)

Product:
Tree Management Graveyard
Component:
TBPL
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(1 file, 3 obsolete files)

Break up the Legend into sections, add key/mouse bindings
16.64 KB, patch
Swatinem
: review+
Details | Diff | Splinter Review
tbpl should have a help page listing the available keyboard shortcuts. Implementation note: _keymap contains a mapping from keys to actions. It does not include mouse actions, though (eg ctrl-click to add a build to the current selection.) See also bug 654908.
Probably related to or a duplicate of bug 579594.
We could add a dropdown menu showing the shortcuts.
OS: Linux → All
Hardware: x86_64 → All
Attached patch Add shortcuts doc dropdown (obsolete) — — Splinter Review
(In reply to comment #2) > We could add a dropdown menu showing the shortcuts. No, we can't, because we'd have to figure out a name for it. :) I was just filing this bug in case somebody else got around to it before me. The attached patch adds a dropdown named "Keyboard", and then puts some non-keyboard documentation in it. But I couldn't come up with a better name. Internally, I call it "shortcuts", which isn't very descriptive either. I think "Keyboard" is good enough, but would not oppose a renaming.
Attachment #532718 - Flags: review?(arpad.borsos)
Comment on attachment 532718 [details] [diff] [review] Add shortcuts doc dropdown Review of attachment 532718 [details] [diff] [review]: ----------------------------------------------------------------- Hm UI wise I’d like to try making the Legend dropdown into a multi-column layout with additional headings. I don’t know how difficult it would be to do that (and if it would look good at all). There would ideally also be a link to the tbpl wiki page (https://wiki.mozilla.org/Tinderboxpushlog) which itself needs a lot of work :-D ::: js/UserInterface.js @@ +42,5 @@ > this._data = controller.getData(); > > + for (var i = 0; i < this._orderedKeyMap.length; i += 2) { > + this._keymap[this._orderedKeyMap[i]] = this._orderedKeyMap[i+1]; > + } I would rather want to avoid this and leave keymap as an object and do a for (var key in keymap) shortcutdoc[keymap[key]] (you already use action = keymap[key] as fallback) That seems nicer than doing this initialization and the i += 2. Or wait? You named it “ordered”. What would the enumeration order be of the standard keymap as it is now?
Oops. I guess a crash ate my comment a while back. The ordering thing is my Javascript naïveté -- I'm used to scripting languages where associative arrays are unordered. After reading up, it appears that this is officially also true of JS, but in practice browsers try to keep things in insertion order and mostly succeed, so the web depends on it. Ok, that's what I had first, and it's much simpler. I'll play with adding it to the Legend. My main concern there is that people are currently trained that the legend is useless, so will never look there for the keyboard shortcuts. :( Maybe I should rename it to "Help" at the same time, to reflect its expanded contents?
Well, here's a fairly straightforward integration of the shortcut docs into the Legend. You can check how it feels.
Attachment #532718 - Attachment is obsolete: true
Attachment #532718 - Flags: review?(arpad.borsos)
Attachment #535908 - Flags: review?(arpad.borsos)
Going further, here's an add-on patch to the previous patch that splits the legend up into shorter lists of builds and tests, and splits out the color key into an initial section. (The current legend goes well below the bottom of my screen.) It still isn't beautiful, but IMHO it's an incremental improvement.
Attachment #535909 - Flags: review?(arpad.borsos)
Comment on attachment 535908 [details] [diff] [review] Add keyboard/mouse section to Legend (renamed to Help) Review of attachment 535908 [details] [diff] [review]: ----------------------------------------------------------------- ::: index.html @@ +11,5 @@ > <body class="noscript"> > > <div id="topbar"> > <div id="config"> > + <div class="helpContainer" class="dropdown"><h2>Help</h2> That double class definition lead to that menu being always open from the start, but I see you fixed that with the second patch. @@ +23,5 @@ > + <td><dl id="shortcuts"></dl></td> > + </tr> > + </table> > + </div> > + <div id="treeInfoContainer" class="dropdown"><h2>Tree Info</h2><dl id="treeInfo" class="dropdownContents"> Markus has cleaned up the html here, that may lead to some merge conflicts. Just so both of you are warned :P ::: js/UserInterface.js @@ +27,5 @@ > + _cmddoc: { 'prev-unstarred': 'Highlight previous unstarred failure', > + 'next-unstarred': 'Highlight next unstarred failure', > + 'toggle-unstarred': 'Toggle showing only unstarred failures', > + 'select': 'Select/deselect active build or changeset', > + 'show-comment': 'Show the comment box (prefilled with<br>selected builds and changesets)' The linebreak because of that long description does not look very good.
Attachment #535908 - Flags: review?(arpad.borsos) → review+
Comment on attachment 535909 [details] [diff] [review] Split legend into builds, tests, and other Review of attachment 535909 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good. I think the rather large empty space to the right can be used like this: Move the wiki link to the right and make it bold, that way it is far more discoverable. r+ with the other nits fixed as well. ::: css/style.css @@ +101,4 @@ > #helpContainer dd { > margin-left: 6em; > position: relative; > } td:not(:last-child) dd needs some kind of right margin/padding so there is a greater gap between the different columns. Also make sure the table has no margin of its own. It seems like the builds column has more space to the left than the color codes. Also, the space below the table is too big. ::: js/Config.js @@ +384,5 @@ > + Config.resultNames[b] = Config.buildNames[b]; > +} > +for (var t in Config.testNames) { > + Config.resultNames[t] = Config.testNames[t]; > +} Not that nice, but we can live with it. ::: js/UserInterface.js @@ +355,5 @@ > '<dt class="exception">purple</dt><dd>infrastructure exception</dd>' + > '<dt class="busted">red</dt><dd>build error</dd>' + > '<dt class="retry">blue</dt><dd>build has been restarted</dd>' + > '<dt class="unknown">black</dt><dd>unknown error</dd>' + > + '').appendTo($('#legend-other')); Move this into index.html.
Attachment #535909 - Flags: review?(arpad.borsos) → review+
Blocks: 579594
(In reply to comment #8) > Comment on attachment 535908 [details] [diff] [review] [review] > @@ +23,5 @@ > > + <td><dl id="shortcuts"></dl></td> > > + </tr> > > + </table> > > + </div> > > + <div id="treeInfoContainer" class="dropdown"><h2>Tree Info</h2><dl id="treeInfo" class="dropdownContents"> > > Markus has cleaned up the html here This will probably land before bug 630537, so I'll be the one who has to merge :-) Anyway, I'm just adding line breaks and changing indentation, nothing special.
Ok, since you're fine with the approach in that second patch, I fixed the problems you identified but took a different approach at fixing the off-kilter layout. What do you think of this one? (I merged the patches.) You can see it at http://people.mozilla.org/~sfink/tinderboxpushlog/
Assignee: nobody → sphink
Attachment #535908 - Attachment is obsolete: true
Attachment #535909 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #536370 - Flags: review?(arpad.borsos)
Comment on attachment 536370 [details] [diff] [review] Break up the Legend into sections, add key/mouse bindings Review of attachment 536370 [details] [diff] [review]: ----------------------------------------------------------------- Awesome! ::: js/UserInterface.js @@ +48,1 @@ > this._buildLegend(); These can be merged into a _buildHelp() in a followup.
Attachment #536370 - Flags: review?(arpad.borsos) → review+
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/f201bc885770 What's the status policy here? Should I RESOLVE things when landing them, and mstange will CLOSE them when updating on tbpl? Or do I just put in the changeset URL? I could put [fixed-in-???] in the whiteboard, but I don't know what to call ???.
So far, our newly changed policy is just put in the URL and hope someone will resolve for you when they pull (a someone who probably won't be mstange, since last I heard he made the mistake of becoming no longer an employee, so he no longer has access to pull). Personally, I think the easiest system is to push, and then start mercilessly hounding ehsan and all of releng until someone updates tbpl.m.o to shut you up, at which point you can just go ahead and resolve. Another option would be to resolve on push, but mark as depending on an "update tbpl.m.o" bug, either an existing one if there's other stuff pending or a newly filed one if you're the first one since the last pull. That way we could stop confusing ourselves about which of our bugs we've pushed, while still having a way to tell interested users how to know when something will be deployed.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: