clarete / hackernews.el

Hacker News client for Emacs
GNU General Public License v3.0
248 stars 26 forks source link

Describe mode and button keymaps human-readably #47

Closed matthijsk closed 6 years ago

matthijsk commented 6 years ago

Provide an overview of the most important key bindings when using describe-mode, similar to the keymap table in README.md.

Output:

RET Open link in default (external) browser.
t   Open link in text-mode browser within Emacs.

n   Move to next title link.
p   Move to previous title link.
TAB Move to next comments count link.
<S-tab> Move to previous comments count link.
m   Load more stories.
g   Reload stories.
f   Prompt user for a feed to switch to.
q   Quit.
basil-conto commented 6 years ago

Thanks, I think this is a good idea despite personally preferring the various built-in and third-party ways by which these bindings are automagically documented and hyperlinked:




Some notes I gathered while looking at this PR, for posterity (and for anyone interested):

Some minor comments/questions on the PR itself:

basil-conto commented 6 years ago

@matthijsk Heads-up: If GitHub allows me to push commits to your branch, given my Collaborator privileges in this project, I will do so first and then merge the PR, rather than merging the PR before pushing my suggested formatting/wording tweaks.

basil-conto commented 6 years ago

Unless you'd like to look into how substitute-command-keys carries out its formatting yourself?

Turns out the 16/32 column widths are hard-coded in the src/keymap.c function describe_command: http://git.savannah.gnu.org/cgit/emacs.git/tree/src/keymap.c#n3050

So, GitHub allowing, I'll just push a commit increasing the indentation roughly to the 16th column with some tab characters.

basil-conto commented 6 years ago

In other words, I'd rather not hard-code the first column's width to accommodate just S-tab.

Sorry, I only later realised that you had, in fact, indented with a tab character. I bumped this to two tab characters for the aforementioned reasons.

basil-conto commented 6 years ago

Feel free to submit another issue/PR if you disagree with any of my follow-up changes.

Thanks again!

matthijsk commented 6 years ago

Thanks, I think this is a good idea despite personally preferring the various built-in and third-party ways by which these bindings are automagically documented and hyperlinked:

I do as well. The self-documenting behaviour of Emacs is just great. C-h m (and C-h f and C-h v) are features I use a lot. This is when I noticed the hackernews-button-browse-internal (t) binding was missing from the keymap. However, just adding \{hackernews-button-map} to the docstring doesn't produce nice output, imo. If you look at the output of describe-mode for an *info* buffer you will see the bindings are described in a similar fashion (see the Info-mode definition in info.el).

Nevertheless, there's no reason to hold up merging the PR for this; I can just follow-up with another commit.

Please do.

the hackernews-button-map command descriptions should say "link at point" instead of just "link"

I agree. Feel free to make the required changes (which you have already done).

basil-conto commented 6 years ago

However, just adding \{hackernews-button-map} to the docstring doesn't produce nice output, imo.

Agreed; I wish there was an option to toggle between the default substitute-command-keys descriptions and human-readable apropos-style summaries like those in this PR.

If you look at the output of describe-mode for an *info* buffer you will see the bindings are described in a similar fashion (see the Info-mode definition in info.el).

Thanks for the pointer.

basil-conto commented 6 years ago

define-derived-mode automagically documents the corresponding keymap if not explicitly done so already, so the existing \{hackernews-mode-map} can be removed from the docstring

Not true; define-derived-mode is overly cautious and only lists the keymap itself if there are zero existing substitute-command-keys sequences, so...

(I intend to do so after merging this PR).

...I no longer intend to do so.