[WIP] footlinks: Add threshold configuration for footlinks. #780
Conversation
|
@preetmishra can you please review this and suggest the changes regarding linting.
|
There was a problem hiding this comment.
@shanukun Thanks for working on this. 👍 It seems to work well with footlinks=enabled. However, we'd likely want to only have footlinks_threshold.
The linting error is a mypy bug. It was triggered due to the double dict unpacking in the Controller instantiation.
The code in run.py doesn't seem to be in-line with how the other existing boolean setting is handled. You might want to read the existing code to explore it further. That said, we could also refactor the existing code if you think we could handle it in a better way.
zulipterminal/core.py
Outdated
| def __init__( | ||
| self, config_file: str, theme: ThemeSpec, color_depth: int, | ||
| autohide: bool, notify: bool, footlinks: bool, footlinks_threshold: int | ||
| ) -> None: | ||
| self.theme = theme | ||
| self.color_depth = color_depth | ||
| self.autohide = autohide | ||
| self.notify_enabled = notify | ||
| self.footlinks_enabled = footlinks |
There was a problem hiding this comment.
We would likely want to only have footlinks_threshold.
zulipterminal/ui_tools/boxes.py
Outdated
| @@ -535,7 +535,13 @@ def footlinks_view( | |||
| return None | |||
|
|
|||
| footlinks = [] | |||
| for link, (text, index, show_footlink) in message_links.items(): | |||
| for limit, message_link in enumerate(message_links.items()): | |||
| # print("Value:", self.model.controller.footlinks_threshold) | |||
There was a problem hiding this comment.
Let's remove the print comment. :)
There was a problem hiding this comment.
Yeah. Definitely.
zulipterminal/ui_tools/boxes.py
Outdated
| if (limit == self.model.controller.footlinks_threshold): | ||
| break | ||
|
|
||
| link, (text, index, show_footlink) = message_link |
There was a problem hiding this comment.
Perhaps iterate over the key, link, and unpack its value here?
zulipterminal/ui_tools/boxes.py
Outdated
| @@ -535,7 +535,13 @@ def footlinks_view( | |||
| return None | |||
|
|
|||
| footlinks = [] | |||
| for link, (text, index, show_footlink) in message_links.items(): | |||
| for limit, message_link in enumerate(message_links.items()): | |||
There was a problem hiding this comment.
limit feels like a misnomer. Would index be better?
There was a problem hiding this comment.
We are already using index for storing message_link index. Using index will not affect the code but it will definitely be confusing.
Maybe pos or position?
There was a problem hiding this comment.
Could we rather not change this line and use the existing index for the check? That would be a lot cleaner and would not require #780 (comment) either.
There was a problem hiding this comment.
I tried that but message_links does not contain all the links from 1 to n maybe because some links (like chat.zulip.org) are indexed but not added to message_links
- Here I set
footlinks_threshold=5but I only got 4 links
There was a problem hiding this comment.
Ah, yes. We skip trivial footlinks to save screen real-estate. The earlier approach would be okay.
| footlinks_threshold = zterm['footlinks_threshold'][0] | ||
| if footlinks_threshold.isnumeric(): | ||
| int_settings['footlinks_threshold'] = int(footlinks_threshold) | ||
|
|
There was a problem hiding this comment.
So I should remove footlinks=enabled setting and keep only footlinks_threshold and also should change all bool settings to int?
There was a problem hiding this comment.
Yes, that's what I was suggesting - remove footlinks and only have footlinks_threshold.
I believe the current set of boolean settings are user-friendly with enabled and disabled. We shouldn't change all the settings to int.
There was a problem hiding this comment.
- I meant storing them in
Dict()asint.
| @@ -321,7 +327,8 @@ def main(options: Optional[List[str]]=None) -> None: | |||
| Controller(zuliprc_path, | |||
| theme_data, | |||
| int(args.color_depth), | |||
| **boolean_settings).main() | |||
| **boolean_settings, | |||
| **int_settings).main() | |||
There was a problem hiding this comment.
See the second point in the review.
|
@shanukun I was meaning to put this in the last review but it slipped my mind! You could include You can read more about Zulip's commit guidelines here: https://zulip.readthedocs.io/en/latest/contributing/version-control.html#commit-messages. |
shanukun
force-pushed
the
enchance-773
branch
2 times, most recently
from
c3eeec4 to
002bbee
Compare
zulipbot
added
size: M
This enables user to configure footlinks threshold settings using footlinks_threshold in zuliprc. Fixes zulip#773.
|
@preetmishra can you please review the changes again. |
|
@shanukun I have not tested the pull request locally, but it appears that we still do not have a way to replicate the 'enabled' mode with the new threshold setting. Let's continue the discussion in #zulip-terminal>Configurable footlinks by threshold before moving ahead with the pull request. |
|
@shanukun @preetmishra Where do we stand on this PR? |
|
@neiljp Primarily, it needs a response from Shanu re the implementation plan that he has as the PR currently doesn't have a way to replicate the original 'enabled' mode, where all the footlinks are shown. |
neiljp
added
the
PR awaiting update
|
@preetmishra Thanks for the summary 👍 I think @shanukun may be contributing elsewhere, but of course is welcome to pick this back up. |
|
@shanukun Thanks for your work on starting this feature 👍 @Abhirup-99 has extended this in #841 so I'm closing this now. |
This enables user to configure footlinks threshold settings
using footlinks_threshold in zuliprc.
#773 .