Config to set /ignore and /notify #344

trevarj · 2021-08-02T07:30:27Z

Added the ability to configure /ignore and /notify defaults for tabs.
The settings can be added under each chan, server or default section.

Overview:

The most specific config will be the highest priority, i.e if you configure ignore under a chan then it will supersede the server's ignore, which will supersede the ignore under defaults:.

Priority hierarchy (highest to lowest):

Server tab: server section -> defaults section -> hardcoded defaults (never ignore, notifier depends on conditional compilation, see impl Default for Notifier)
Chan tabs: chan section -> server section -> defaults section -> current setting on server tab (maybe someone toggled the setting manually)
User tabs: never ignore, notifier is always Messages (pre-existing behavior)

If acceptable, closes #118

crates/tiny/src/config.rs

osa1

Thanks @trevarj . Just did a quick pass, it looks great.

No support for /join or /connect command args

I wonder if we should also do that. We will need to implement command args anyway, and if we do that today the fields ignore and notify will be redundant, so perhaps we could just rely on command args, instead of having two ways of doing the same thing in the config (which raises questions like which one will have priority when in conflict).

If we can design a good syntax for command args then it should be a few lines of change in the config parser. We could use the same types and rest of the code.

crates/libtiny_tui/src/config.rs

trevarj · 2021-08-06T06:37:02Z

@osa1 my problem with the command args approach was that it's not straightforward going from cmd.rs all the way to the UI with the current way we handle auto-joining channels. I believe we would have to keep UI specific configs in State, which seemed slightly hacky to me at the time. Maybe you know a better way, or if that is acceptable?

crates/tiny/src/cmd.rs

crates/tiny/src/config.rs

trevarj · 2021-08-07T20:13:02Z

I was able to add parsing for /join and switched the config's join: to an identical string, i.e - "#tiny -ignore", but ran into an issue...
After adding a new UI function, set_tab_config(..), which I wanted to call from in cmd.rs after client.join(), I realized that there's an annoying race.

/join #tiny -ignore is entered
parse the command args, call client.join() as normal
call ui.set_tab_config() to set the proper ignore value https://github.com/trevarj/tiny/blob/default-ignore-notify-v2/crates/tiny/src/cmd.rs#L339
Turns out ui.set_tab_config() runs through, then gets clobbered by the client joining and creating a new tab. Doesn't matter if I switch the ordering or set set_tab_config() to can_create_tab

Need to think about this more I guess. Branch is here: https://github.com/trevarj/tiny/tree/default-ignore-notify-v2

trevarj · 2021-08-15T11:30:53Z

From most recent commits:

When joining a channel using the /join command, the tab is created
before the server is actually joined so we can effectively set the
channel config (ex. ignore the tab, set notifier).
Due to this, I had to add handling for REPLY messages that occur when a
channel cannot be joined but the tab has already been created. Now an
error message shows on the chan tab, giving the reason why the chan
can't be joined.

@osa1 let me know if you get some time to try this out and can give some thoughts on the UX here.

osa1

Thanks @trevarj . Just one quick pass for now, will review again in detail later. It would be good to revert unrelated changes in the meantime.

One question re: PR description:

Chan tabs: chan section -> server section -> defaults section -> current setting on server tab (maybe someone toggled the setting manually)

Shouldn't current tab setting have highest priority?

crates/libtiny_tui/src/config.rs

crates/libtiny_tui/src/tui.rs

crates/libtiny_tui/src/config.rs

crates/tiny/config.yml

crates/tiny/src/cmd.rs

crates/tiny/src/conn.rs

osa1

We should document how overriding notify and ignore (e.g. tab ignore overrides server and default). I'll post a suggestion here.

osa1 · 2021-09-12T06:31:10Z

crates/libtiny_tui/src/config.rs

+            format!("#{}", s)
+        } else {
+            s.to_string()
+        };


Not a big deal probably, but I wonder if we should expect user to get this right instead of inserting a # for them. The problem is channel names can contain multiple #s, not just one, but we assume one here.

This is something I noticed other clients do, so I added it if there is no #. It seems like a nicer UX, but we don't need to have it.

crates/libtiny_tui/src/config.rs

crates/tiny/src/conn.rs

Raphalex46 · 2022-12-13T20:06:38Z

Hi, is this PR still going to be merged ? I was looking to know if it was possible to set notify in the config.yml and arrived here. If you need help with anything I can take a look. Being able to set default configuration values in the configuration file could be a very useful feature.

Added the ability to configure /ignore and /notify defaults for tabs. The settings can be added under each chan, server or default section.

When joining a channel using the /join command, the tab is created before the server is actually joined so we can effectively set the channel config (ex. ignore the tab, set notifier). Due to this, I had to add handling for REPLY messages that occur when a channel cannot be joined but the tab has already been created. Now an error message shows on the chan tab, giving the reason why the chan can't be joined.

When an auto-join channel cannot be joined, it will send a client error message, so we open the tab and display the error message to the user.

trevarj · 2022-12-13T21:37:22Z

@Raphalex46 I've rebased with master to fix the conflicts. If you'd like you can test this out and see if it works as you expect. I have been using it for a long time now and it works well for me. See here for config details

Raphalex46 · 2022-12-13T21:58:44Z

I've tested it a little bit, the only that I didn't manage to get working is the -notify off option.

trevarj · 2022-12-14T11:52:20Z

@Raphalex46 I made another commit with some fixes. You can do /reload to reload the configs.
Make sure you compiled with features = desktop-notifications.

Raphalex46 · 2022-12-14T13:17:02Z

I can't test it right now but I will as soon as I can and I'll let you know then.
Yes it was compiled with features=desktop-notifications, I just didn't manage to get the
notifications to "off" by default.

trevarj · 2022-12-14T13:27:08Z

I just didn't manage to get the notifications to "off" by default

Right, should work now.

Raphalex46 · 2022-12-14T19:07:32Z

Yup, seems to work pretty much as expected. I suppose we have to wait to see if @osa1 is ok with merging this.

I was also thinking, eventually, options like these could be expressed in the config file like this:

join:
  - name: "#tiny"
    ignore: true
    notify: messages

right ?

That way, configuration defaults could even be expressed for a whole server:

servers:
  - addr: irc.oftc.net
    port: 6697
    # ...
    ignore: true
    notify: messages

trevarj · 2022-12-14T19:29:33Z

@Raphalex46 thanks a lot for taking the time to test.

I was also thinking, eventually, options like these could be expressed in the config file like this:

The reason why they are using a special parsing was so that it could be leveraged for on-the-fly /connect or /join, e.g if you want to join a chan and have it ignored in one shot (kinda pointless, but saves a click...). That actually works now, if you do /help you should see it in the help message for /join.

That way, configuration defaults could even be expressed for a whole server:

This should actually be working too.
From this test in the code you can see a simple example. The precedence is chan -> server -> defaults.

If you create a window with /connect it should use the configs under the server, or fallback to defaults.
If you create a window with /join it should use the config of the chan, fallback to the current server tab config (may be modified on the fly by user), and then defaults.

I will write more thorough docs in the example config.yaml once osa1 gives the 👌🏻.
Bit lazy to do it right now especially since this PR was sitting for over a year!!

Raphalex46 · 2022-12-14T19:53:06Z

Yes, I saw that you can join like that, and it totally justifies the parsing!

I also did not see that you could put defaults like that for servers in the config file, so thanks for pointing that out.

I just mentioned this because we have a YAML config file, and it seemed more satisfying to use that for the default config of the channels. You're right that this PR probably isn't the place to implement this, just wanted to put it out there before I forgot :)

osa1

Thanks again @trevarj!

Added some inline comments.

There are lots of formatting changes in the PR, could you revert those please?

I know we're inconsistent in ordering of imports, but for now if you could follow the conventions in files to keep the diff small that would be really appreaciated. We can then do a pass to format them if we want.

(I'll add a few more things)

crates/tiny/src/cmd.rs

crates/tiny/src/conn.rs

crates/libtiny_tui/src/tests/config.rs

Moved from osa1#344 into separate PR. Information on what this does can be found in this comment: https://github.com/osa1/tiny/pull/344/files#r706813943

osa1

As discussed in #397:

We should store all tab settings in TUI settings so that when we need to create a tab we will be able to find the settings for the tab.

We still need to set tab settings on creation, to be able to implement /join ... -ignore etc. commands.
Design question: if I update settings of a tab, then close it, and create the tab again, should I get the settings in the config or the updated settings?
- Option (1) Use the updated settings: We should not store the tab settings in Tab, get the settings from the config every time we need it.
- Option (2) Use the settings from the config file: We should store the tab settings in Tab
- Currently notification settings are stored in Tab, but for ignore we use the config. This needs to be consistent: either use both from the config (implements (1)), or store both in Tab (implements (2)).

I don't feel strongly, but I think (1) is probably better.

trevarj · 2022-12-15T11:12:47Z

Design question: if I update settings of a tab, then close it, and create the tab again, should I get the settings in the config or the updated settings?

Let's just go with option (1) so that the previous config for the tab is restored from tab_configs on TUI (which was sourced from the config file at startup). This seems like an unusual edge case anyway.

I will make an attempt at the following:

Move tab config state (notifier, status/ignore) out of Tab and only have TabConfigs on TUI
Remove all formatting changes
Use proper yaml for chan config under join: (if we change join it will break everyone's config)

- Removed ignore and notifier configs from `Tab` - Use proper YAML for `Chan` config - Reverted formatting changes

trevarj · 2022-12-15T19:27:37Z

The above checklist is done, albeit with a bit of a hack to resolve an issue that I had that comes from moving ignore and notifier off of Tab (has to do with toggling if you look into the code and last commit message).

This has lost its fun for me, so if anyone wants to help, now is the time 😩

Fixed toggling the tab config (/ignore and /notify) when on a server tab and also for when on a user tab. The hack is how we are storing user nicks as channel names in TabConfig. That can perhaps be refactored later to look nicer.

Raphalex46 · 2022-12-15T21:18:59Z

I will try to take a look when I have the time, probably this week-end.

osa1 · 2022-12-16T07:09:43Z

Thanks @trevarj, I think this looks pretty good. Let's test this a little bit (the parts without unit tests) and merge. I will update README and CHANGELOG.

mizlan · 2023-06-27T00:51:20Z

Is there anything blocking merge?

osa1 · 2023-06-29T13:19:06Z

Sorry for the delay. I'll try to merge this this week.

trevarj commented Aug 2, 2021

View reviewed changes

crates/tiny/src/config.rs Show resolved Hide resolved

osa1 reviewed Aug 6, 2021

View reviewed changes

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

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

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

osa1 reviewed Aug 6, 2021

View reviewed changes

crates/tiny/src/cmd.rs Outdated Show resolved Hide resolved

osa1 reviewed Aug 6, 2021

View reviewed changes

crates/tiny/src/config.rs Outdated Show resolved Hide resolved

osa1 reviewed Sep 9, 2021

View reviewed changes

trevarj force-pushed the default-ignore-notify branch from 15e1879 to 0f7b9ed Compare September 9, 2021 18:09

osa1 reviewed Sep 12, 2021

View reviewed changes

osa1 mentioned this pull request Sep 12, 2021

Implemented DCC get for downloading files. #209

Open

trevarj mentioned this pull request Sep 16, 2022

hide join/leave messages? #392

Closed

trevarj added 10 commits December 14, 2022 00:22

Config to set /ignore and /notify

2b1c86a

Added the ability to configure /ignore and /notify defaults for tabs. The settings can be added under each chan, server or default section.

Allow add_client_err_msg to create tab

8c77758

When an auto-join channel cannot be joined, it will send a client error message, so we open the tab and display the error message to the user.

Remove unused Clone

07d57f1

Fix config example for join field

cf98be0

Revert unrelated refactoring

c101f6b

Use [..] syntax in config arg examples

55f5058

Iterator simplifications

baec639

Revert unrelated visibility change

79dcbe3

Large refactor / cleanup. Needs testing

6c4eb09

trevarj force-pushed the default-ignore-notify branch from 0f7b9ed to 6c4eb09 Compare December 13, 2022 21:34

trevarj added 2 commits December 14, 2022 14:47

some fixes to get notifier and /reload working

43d4424

formatting

123d2fe

fix test error

c0c0e73

osa1 reviewed Dec 15, 2022

View reviewed changes

crates/tiny/src/cmd.rs Outdated Show resolved Hide resolved

crates/tiny/src/conn.rs Outdated Show resolved Hide resolved

crates/libtiny_tui/src/tests/config.rs Outdated Show resolved Hide resolved

trevarj added a commit to trevarj/tiny that referenced this pull request Dec 15, 2022

Added handling for more ERR Replies

2e64913

Moved from osa1#344 into separate PR. Information on what this does can be found in this comment: https://github.com/osa1/tiny/pull/344/files#r706813943

trevarj mentioned this pull request Dec 15, 2022

Added handling for more ERR Replies #397

Closed

osa1 reviewed Dec 15, 2022

View reviewed changes

Changes based on PR conversation

7a4a09e

- Removed ignore and notifier configs from `Tab` - Use proper YAML for `Chan` config - Reverted formatting changes

Add hacky logic for toggling tab config

f673fd8

Fixed toggling the tab config (/ignore and /notify) when on a server tab and also for when on a user tab. The hack is how we are storing user nicks as channel names in TabConfig. That can perhaps be refactored later to look nicer.

trevarj force-pushed the default-ignore-notify branch from af4a8fa to f673fd8 Compare December 15, 2022 19:41

revert another formatting change

1c60171

Merge branch 'master' into default-ignore-notify

a1e6d1d

osa1 merged commit f73716a into osa1:master Jun 29, 2023

osa1 deleted the default-ignore-notify branch June 29, 2023 20:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config to set /ignore and /notify #344

Config to set /ignore and /notify #344

trevarj commented Aug 2, 2021 •

edited

Loading

osa1 left a comment

trevarj commented Aug 6, 2021

trevarj commented Aug 7, 2021

trevarj commented Aug 15, 2021

osa1 left a comment

osa1 left a comment

osa1 Sep 12, 2021

trevarj Sep 12, 2021

Raphalex46 commented Dec 13, 2022

trevarj commented Dec 13, 2022

Raphalex46 commented Dec 13, 2022

trevarj commented Dec 14, 2022

Raphalex46 commented Dec 14, 2022

trevarj commented Dec 14, 2022

Raphalex46 commented Dec 14, 2022

trevarj commented Dec 14, 2022

Raphalex46 commented Dec 14, 2022

osa1 left a comment

osa1 left a comment

trevarj commented Dec 15, 2022 •

edited

Loading

trevarj commented Dec 15, 2022 •

edited

Loading

Raphalex46 commented Dec 15, 2022

osa1 commented Dec 16, 2022

mizlan commented Jun 27, 2023

osa1 commented Jun 29, 2023

Config to set /ignore and /notify #344

Config to set /ignore and /notify #344

Conversation

trevarj commented Aug 2, 2021 • edited Loading

Priority hierarchy (highest to lowest):

osa1 left a comment

Choose a reason for hiding this comment

trevarj commented Aug 6, 2021

trevarj commented Aug 7, 2021

trevarj commented Aug 15, 2021

osa1 left a comment

Choose a reason for hiding this comment

osa1 left a comment

Choose a reason for hiding this comment

osa1 Sep 12, 2021

Choose a reason for hiding this comment

trevarj Sep 12, 2021

Choose a reason for hiding this comment

Raphalex46 commented Dec 13, 2022

trevarj commented Dec 13, 2022

Raphalex46 commented Dec 13, 2022

trevarj commented Dec 14, 2022

Raphalex46 commented Dec 14, 2022

trevarj commented Dec 14, 2022

Raphalex46 commented Dec 14, 2022

trevarj commented Dec 14, 2022

Raphalex46 commented Dec 14, 2022

osa1 left a comment

Choose a reason for hiding this comment

osa1 left a comment

Choose a reason for hiding this comment

trevarj commented Dec 15, 2022 • edited Loading

trevarj commented Dec 15, 2022 • edited Loading

Raphalex46 commented Dec 15, 2022

osa1 commented Dec 16, 2022

mizlan commented Jun 27, 2023

osa1 commented Jun 29, 2023

trevarj commented Aug 2, 2021 •

edited

Loading

trevarj commented Dec 15, 2022 •

edited

Loading

trevarj commented Dec 15, 2022 •

edited

Loading