Enhanced /help command #340

trevarj · 2021-07-04T15:43:01Z

Entering /help will now spawn a new "dummy" tab that shows all the
possible commands, along with all the currently configured key bindings.

Closes #287

Entering /help will now spawn a new "dummy" tab that shows all the possible commands, along with all the currently configured key bindings. Closes osa1#287

trevarj · 2021-07-04T15:43:23Z

Looking for some feedback/suggestions here.

trevarj · 2021-07-04T15:47:49Z

Here is how the "help" tab currently looks with this patch. You can scroll up and down and /close it.

My test config has some duplicate actions. I think this might look better if the action is listed once with the keys separated by commas, but I didn't get around to doing that yet.

Commands:
/away       - Sets/removes away message - Usage: `/away` or `/away <message>`
/clear      - Clears current tab        - Usage: `/clear`
/close      - Closes current tab        - Usage: `/close`
/connect    - Connects to a server      - Usage: `/connect <host>:<port>` or `/connect` to reconnect
/help       - Displays this message     - Usage: `/help`
/ignore     - Ignore join/quit messages - Usage: `/ignore`
/join       - Joins a channel           - Usage: `/join <chan1>,<chan2>,...` or `/join` in a channel tab to rejoin
/me         - Sends emote message       - Usage: `/me <message>`
/msg        - Sends a message to a user - Usage: `/msg <nick> <message>`
/names      - Shows users in channel    - Usage: `/names`
/nick       - Sets your nick            - Usage: `/nick <nick>`
/notify     - Set channel notifications  - Usage: `/notify [off|mentions|messages]`
/quit       - Quit tiny                 - Usage: `/quit`
/reload     - Reloads config file         - Usage: `/reload`
/switch     - Switches to tab           - Usage: `/switch <tab name>`
Key Bindings:
Cancel               - Esc
Exit                 - Ctrl('c')
Exit                 - Ctrl('d')
InputAutoComplete    - Tab
InputDeleteNextChar  - Del
InputDeletePrevChar  - Backspace
InputDeletePrevWord  - Ctrl('w')
InputDeleteToEnd     - Ctrl('k')
InputDeleteToStart   - Ctrl('u')
InputMoveCursEnd     - Ctrl('e')
InputMoveCursEnd     - End
InputMoveCursLeft    - Arrow(Left)
InputMoveCursRight   - Arrow(Right)
InputMoveCursStart   - Ctrl('a')
InputMoveCursStart   - Home
InputMoveWordLeft    - CtrlArrow(Left)
InputMoveWordRight   - CtrlArrow(Right)
InputNextEntry       - Arrow(Down)
InputPrevEntry       - Arrow(Up)
InputSend            - Char('\r')
MessagesPageDown     - PageDown
MessagesPageUp       - PageUp
MessagesScrollDown   - ShiftArrow(Down)

osa1

Great work @trevarj , thanks! I'm surprised by how simple this is.

I think this line needs to be updated:

tiny/crates/tiny/src/cmd.rs

Line 297 in 8e7a68a

if serv == "mentions" {

Does this have any problems with showing connection events in help tab? I'm guessing it works exactly the same way as "mentions" tab?

crates/libtiny_tui/src/tui.rs

trevarj · 2021-08-06T06:47:10Z

Does this have any problems with showing connection events in help tab? I'm guessing it works exactly the same way as "mentions" tab?

Yeah, it's identical. I will update that line, thanks for catching it

…-help

osa1 · 2021-08-17T09:47:09Z

I thought I sent this comment but apparently I didn't.. This looks great but one problem I had is it switches to the help tab, which is fine, but when I close the tab I'm not back in the tab I was before /help. This is quite annoying to me as a user. Firefox handles this nicely. If you create a new tab with C-t and then close it with C-w you're back in the tab before doing C-t. It would be very annoying if it instead moved you to the tab left to the new tab created with C-t.

@trevarj WDYT?

trevarj · 2021-08-17T10:02:35Z

@osa1 seems like that's a general feature of the tui.

I can store previous_tab on the tui and always go back to it when a tab is closed on this PR, or make a new issue and address it. Up to you.

osa1 · 2021-08-17T10:08:12Z

Yeah I think storing the previous tab and switching to it when closing a tab would fix the issue. I'm trying to think if it would be useful in any other case. I just tried this in Firefox and it seems to use a similar idea. If I move left and close the tab the tab on the right is selected. If I move right and close the tab the tab on the left is selected. However, if I jump to a specific tab with alt+<number> then close it, I'm moved one next to it, instead of the active tab before jumping. I'm not sure if I like this, though I didn't even think about this until now so clearly I don't care too much. We don't have to duplicate this behavior, but I'm just trying to understand what will be the most intuitive.

trevarj · 2021-08-17T10:39:40Z

@osa1 I think it should probably just get logged as another issue since we will need to decide if:

we want to keep a history stack of all previously visited tabs and maintain it when tabs are closed
we only want to keep track of the last tab and then fall back to the nearest tab (current behavior)
or simply don't move to the /help tab 😁

Any way seems easy to implement

osa1 · 2021-08-28T16:33:29Z

@trevarj another idea:

Make help tab move freely in and out of "groups", for example if my tab bar looks like
```
mentions server1 #channel1 server2 #channel2 help
```
If I move help tab to the left it should look like
```
mentions server1 #channel1 server2 help #channel2
```
One more:
```
mentions server1 #channel1 help server2 #channel2
```
and so on.
Create help tab next to the current tab, so when I close it I'm back to where I was.

This seems to solve the problem of losing current tab after /help and then closing the help tab without adding any extra state in TUI (previous tab).

This way it gets added next to the tab that you are currently on.

trevarj · 2021-08-28T17:02:39Z

@osa1 I made "help" a chan tab instead of a "serv" tab so that it opens next to the current tab you're on.

osa1 · 2021-08-28T17:21:45Z

It doesn't seem to work correctly. If I do /help in a server tab the tab created as the last tab in the group.

Also, I'm not sure if creating as a channel tab a good idea. I can't move it past groups, as mentioned in my comment above.

I wonder if we should make MsgSource optional in tabs. "mentions" and "help" tabs doesn't have MsgSources conceptually (they don't belong to a server). If we do that then we can allow freely moving tabs without MsgSources, and possibly remove some hard-coded checks for mentions and help tabs and check whether MsgSource is available instead.

Added a new "Misc" tab type that will genericize "mentions" and "help" tab, and what ever else we want to add. This type of tab gets created to the right of the current tab and came move freely around all tabs. Moving a server tab captures a misc tab in its group.

crates/libtiny_tui/src/tui.rs

osa1

Thanks @trevarj , this looks great now. I'll make another pass hopefully later today 😅

crates/libtiny_common/src/lib.rs

crates/libtiny_logger/src/lib.rs

Removed "errors" misc tab

trevarj · 2021-08-30T07:36:44Z

@osa1 ok, I think we're really close!! hopefully this is the last commit

osa1

I found the names show_mentions_tab and show_help_tab a bit confusing. To me the name "show..." suggests showing something that already exists, but they also create the tab if they don't exist.

I suggest renaming them to "add_help_tab" or "create_help_tab", and then if we do the switching in the command implementation we don't have to explain (document) to the user that "add_help_tab" will also switch to the tab.

Re: switching to the help tab. I suggest not switching to a misc tab in the tab creating functions, as in general we may not want to switch to a misc tab on creation always (e.g. I think for "errors" tab it's fine to just highlight it with bold red, no need to switch to it). It's specific to the tab.

So I'd completely separate tab creating and switching logic and do switching in command implementation.

crates/libtiny_logger/src/lib.rs

crates/libtiny_tui/examples/bench.rs

osa1 · 2021-08-30T11:54:33Z

crates/libtiny_tui/src/key_map.rs

@@ -152,6 +153,10 @@ impl KeyMap {
    pub(crate) fn load(&mut self, key_map: &KeyMap) {
        self.0.extend(key_map.0.iter())
    }
+
+    pub(crate) fn iter(&self) -> Iter<Key, KeyAction> {


Doesn't matter too much for this code, but you could return impl Iterator<Item = (&Key, &KeyAction)> to avoid referring to the concrete type and leaking details of the type to users.

An example of where this matters: map type is coming from a third-part and users are not part of the project. In that case user crates will have to depend on the map crate.

Since libtiny_tui is only used by tiny and this is std map it's OK in this case.

Thanks, I didn't think of that. Will keep it in mind when/if I'm creating public crates

osa1 · 2021-08-30T11:57:46Z

crates/libtiny_tui/src/tui.rs

@@ -626,6 +643,55 @@ impl TUI {
        tab.set_cursor(cursor);
    }

+    /// Shows "mentions" tab at the beginning of the tab list
+    pub(crate) fn show_mentions_tab(&mut self) -> Option<usize> {


Could you document the return value please? It's a bit unexpected. AFAIU it returns the index if a tab is created. Otherwise returns None. I'm guessing the index is needed to switch to the tab, but I don't know why we want to switch to the tab only if it's just created but not if it already exists but we add a new message. I think in both cases we either want to switch or we don't, so the return value should be usize. Am I missing anything?

The issue is mainly because we can now /close the mentions tab and it can be recreated in maybe_create_tab when you are mentioned...Now that I think about it, it might just be better to not allow /close for the mentions tab. Thoughts?

I just tried and I cannot close mentions tab in this branch. It seems like it's accidentally working?

I think it makes sense to not allow closing mentions tab because we rely on having at least one tab available in a few places, and changing all those places will require more design (for example, how to enter commands after closing all tabs) and implementation work.

crates/libtiny_tui/src/tui.rs

crates/tiny/src/ui.rs

crates/libtiny_tui/src/tui.rs

osa1 · 2021-08-30T12:20:13Z

crates/libtiny_tui/src/tui.rs

+                Some(tab_idx)
+            }
+            Some(idx) => {
+                self.select_tab(idx);


This is very confusing. Switching to the help tab happens in two entirely different places.

Here when tab already exists

In show_help_tab when tab doesn't already exist

Need to refactor this somehow. I have a suggestion in my top-level comment.

I removed the automatic selection fully. Instead, I made the help tab highlight on creation.

Why? If I run /help I want to see help right now, without any more effort. Why run /help if you don't want to see it?

Why not switch to the tab in command implementation?

- Don't select Misc tab automatically - Don't allow closing mentions tab - Added doc comments - Cleanups

- Automatic switching to help tab on /help

osa1 · 2021-08-30T19:11:52Z

crates/tiny/src/cmd.rs

@@ -159,6 +158,9 @@ fn close(args: CmdArgs) {
        MsgSource::User { serv, nick } => {
            ui.close_user_tab(&serv, &nick);
        }
+        // Don't allow closing the mentions tab


@trevarj you disallowed closing mentions tab here.

Are you ok with being able to close it?

No, see my previous comment.

osa1 · 2021-08-30T19:14:59Z

crates/tiny/src/cmd.rs

+                        "/names only supported in chan tabs",
+                        &MsgTarget::CurrentTab,
+                    );
+                }


This diff should be much smaller if you implement this in the style of previous version:

let serv_name = match src.serv_name() { None => return, Some(serv_name) => serv_name, };

Then the lines starting after let client = ... should not change at all.

osa1 · 2021-08-30T19:21:48Z

crates/tiny/src/cmd.rs

+        })
+        .collect::<Vec<_>>();
+    ui.create_help_tab(&msgs);
+    ui.switch(MiscTab::Help.display());


I think this may break if we have a tab for a user with nick "help".

osa1 · 2021-08-30T19:36:02Z

I suggest changing this a little bit which will avoid problems like having a tab for a user with nick "help" and simplify the design quite a lot.

Remove close_misc_tab. Make TUI handle /close if the tab is a "misc" tab. Otherwise the command should be returned to the caller as before.
Add a TUI::show_help method. It should create a tab, add the help message, and switch to the tab. Important thing is to the caller it's now known how the help message is shown. It could be in the current tab, in an overlay, or in a new tab.
In TUI when an Input event received check if the tab is "help". If so, show the error message as you do in this PR, and ignore the event.
Once these are all implemented, I believe we no longer need the "misc tab" stuff, because tiny (the TUI event loop) will never receive an event with the help tab as the source.

I know I asked for too many changes already, sorry for that. I think this design will be much better. The key point is handling the help tab is entirely up to the TUI implementation, rest of the program does not need to know about it. We can also easily change how we show help in the future. With this I think only the files in libtiny_tui should change. The PR should not have any other changes.

Does this make sense?

Why not do the same for the mentions tab? I think maybe we could simplify some of the code by with a similar idea, but what's shown in mentions is decided by tiny (it searches every message for the current nick), so we it needs to know about the tab and manipulate it.

Switch to tab by source instead of by display name.

osa1 · 2021-08-31T06:52:40Z

Just to clarify my thought process here: showing a "help" dialogue (or a tab in our case) in a UI implementation should happen entirely in the UI implementation, should not effect elsewhere in the program. In our case, updating MsgSource is going against it. The plan I described above should make "help" tag an implementation detail of in the TUI and should not change interfaces (MsgSource, TUI API) in any way.

Looking at me previous message above

Add a TUI::show_help method. It should create a tab, add the help message, and switch to the tab. Important thing is to the caller it's now known how the help message is shown. It could be in the current tab, in an overlay, or in a new tab.

I think we could handle /help in TUI and avoid this as well. We can pass the help string (or details needed to create a help string) to TUI on initialization and TUI can show it in /help.

trevarj · 2021-08-31T09:25:24Z

Perhaps I'm being stubborn (or too tired to take another shot at this), but it might become messy since creating a new tab will require MsgSource to be optional. A lot of the functionality of tui.rs requires using a tab's MsgSource. Seems clunky to make Tab into an enum to support "help" tab functionality separately (no src but having a name/alias, having the ability to move freely between other tabs, etc), but also fragile to make "help" a server tab that has some exclusive behavior.

Maybe just keep current functionality (flood current tab with help), and simply the key mappings? 😬

osa1 · 2021-08-31T09:37:55Z

it might become messy since creating a new tab will require MsgSource to be optional

There's only 4 uses of TUI::new_tab! MsgSource has a lot more uses (ag --rust -w MsgSource returns 84 occurrences) so changing TUI::new_tab should be much less messy than adding one more variant to MsgSource.

A lot of the functionality of tui.rs requires using a tab's MsgSource

So the idea is most of these functions should just show the error message ("change tab to a server/chan/user") and return.

Seems clunky to make Tab into an enum to support "help" tab functionality separately

We shouldn't need to turn Tab into an enum, that would be a large refactoring and I don't think it will buy us much. Probably make things worse.

Maybe just keep current functionality (flood current tab with help), and simply the key mappings?

This would be fine too. I don't feel strongly either way.

To rephrase, MsgSource is a quite central type used everywhere. It doesn't make sense to add a variant to it for TUI-specific stuff. All we can do with that variant outside of TUI is to ignore it. Thinking about this more, I think it might make sense to move mentions tab stuff to TUI as well. TUI already knows the current nick so it can search in incoming messages for mentions and update the tab. No need for rest of the client to know about mentions tab. Same idea.

trevarj · 2021-08-31T10:08:24Z

There's only 4 uses of TUI::new_tab

I meant that you actually can't create a Tab without a source, so when you change src: Option<MsgSource> a lot of matching on Some() will occur in other functions around tui.rs. Is that ok?

        self.tabs.insert(
            idx,
            Tab {
                alias,
                widget: MessagingUI::new(
                    self.width,
                    self.height - 1,
                    status,
                    self.scrollback,
                    self.msg_layout,
                ),
                src,
                style: TabStyle::Normal,
                switch,
                notifier,
            },
        );

trevarj · 2021-08-31T17:37:57Z

WIP:
https://github.com/osa1/tiny/compare/master...trevarj:help-tab?expand=1

osa1 · 2021-09-01T07:25:03Z

Thanks @trevarj . If you could submit a PR it will be easier to find. You can create a PR as draft or mark it as draft after creating.

Enhanced /help command

e58506b

Entering /help will now spawn a new "dummy" tab that shows all the possible commands, along with all the currently configured key bindings. Closes osa1#287

Merge branch 'master' into enhanced-help

8e7a68a

trevarj marked this pull request as ready for review August 2, 2021 07:35

osa1 approved these changes Aug 6, 2021

View reviewed changes

crates/libtiny_tui/src/tui.rs Outdated Show resolved Hide resolved

trevarj added 2 commits August 6, 2021 10:17

PR feedback

d6bf457

Merge branch 'enhanced-help' of github.com:trevarj/tiny into enhanced…

5ee74bb

…-help

Make help tab a chan instead of server

217f881

This way it gets added next to the tab that you are currently on.

trevarj force-pushed the enhanced-help branch from 16d5bdf to 0a00b38 Compare August 29, 2021 07:48

trevarj commented Aug 29, 2021

View reviewed changes

crates/libtiny_tui/src/tui.rs Outdated Show resolved Hide resolved

Changed map_or to if-statement

7f5f024

osa1 reviewed Aug 30, 2021

View reviewed changes

crates/libtiny_common/src/lib.rs Outdated Show resolved Hide resolved

crates/libtiny_common/src/lib.rs Outdated Show resolved Hide resolved

crates/libtiny_logger/src/lib.rs Outdated Show resolved Hide resolved

Add MiscTab enum instead of strings

dde0839

Removed "errors" misc tab

Small cleanups

e4bd1df

osa1 reviewed Aug 30, 2021

View reviewed changes

trevarj added 2 commits August 30, 2021 16:50

PR feedback

384bd9d

- Don't select Misc tab automatically - Don't allow closing mentions tab - Added doc comments - Cleanups

Expose switch() on UI

20714a0

- Automatic switching to help tab on /help

osa1 reviewed Aug 30, 2021

View reviewed changes

Change switching to help tab

479a878

Switch to tab by source instead of by display name.

trevarj closed this Aug 31, 2021

osa1 mentioned this pull request Sep 15, 2021

Move "mentions" tab stuff to TUI #359

Open

Enhanced /help command #340

Enhanced /help command #340

Conversation

trevarj commented Jul 4, 2021

trevarj commented Jul 4, 2021

trevarj commented Jul 4, 2021 • edited Loading

osa1 left a comment

Choose a reason for hiding this comment

trevarj commented Aug 6, 2021

osa1 commented Aug 17, 2021

trevarj commented Aug 17, 2021

osa1 commented Aug 17, 2021

trevarj commented Aug 17, 2021

osa1 commented Aug 28, 2021

trevarj commented Aug 28, 2021

osa1 commented Aug 28, 2021

osa1 left a comment

Choose a reason for hiding this comment

trevarj commented Aug 30, 2021

osa1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osa1 commented Aug 30, 2021

osa1 commented Aug 31, 2021

trevarj commented Aug 31, 2021

osa1 commented Aug 31, 2021

trevarj commented Aug 31, 2021

trevarj commented Aug 31, 2021

osa1 commented Sep 1, 2021

trevarj commented Jul 4, 2021 •

edited

Loading