Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new mode for statusbar tab behaviour #1912

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

H3rnand3zzz
Copy link
Contributor

@H3rnand3zzz H3rnand3zzz commented Nov 6, 2023

Display current pages dynamically so users can navigate easily in-depth.

  • Indicate unread message to the left ([<]) and to the right ([>]) of the range.

How to test the functionality

  • Use the following commands (for easier testing, you can add them to scripts/msg folder and /account set YOUR_ACCOUNT startscript msg (BEWARE: can cause occasional strophe-related crash, use 10-15 windows for safety and make /statusbar maxtabs 5 to see the effect better)
/msg 1
/msg 3
/msg 4
/msg 5
/msg 6
/msg 7
/msg 8
/msg 9
/msg 10
/msg 11
/msg 12
/msg 13
/msg 14
/msg 15
/msg 16
/msg 17
/msg 18
/msg 19
  • navigate through pages easily
  • use /statusbar hide name, /statusbar show name, /statusbar hide number to test all the possible mods.

Potentially fixes #1764.
Fixes #1283

Before the change

Win 6 (or 7, I don't know and remember and have no way to know except manually counting)

I am on tab 6 (or 7, I don't know and remember and have no way to know except manually counting)

2 tabs more left to the previous position

I am 2 tabs more left to the previous position

Win 3

I am on tab 3

After the change

Win 6

image

Win 8 with new message in console

image

Win 2 with new message in console

image

Win 4 with new message in console and from a "friend"

image

@H3rnand3zzz H3rnand3zzz changed the title Add dynamic paging to statusbar Add dynamic paging to statusbar [low priority] Nov 6, 2023
@H3rnand3zzz H3rnand3zzz force-pushed the feature/dynamic-statusbar branch 2 times, most recently from 5904e71 to a6e2aa7 Compare November 7, 2023 17:02
@H3rnand3zzz H3rnand3zzz changed the title Add dynamic paging to statusbar [low priority] Add dynamic paging to statusbar Nov 8, 2023
@H3rnand3zzz
Copy link
Contributor Author

I will provide comments here for each rebase to keep the context. Previous rebases were with master, just to keep the code up-to-date.

@H3rnand3zzz H3rnand3zzz force-pushed the feature/dynamic-statusbar branch 2 times, most recently from f213499 to 5b6481d Compare November 8, 2023 09:59
@H3rnand3zzz
Copy link
Contributor Author

Revision 1

  • Add function to count digits in numbers
  • Add function to count digits in range of numbers
  • Indicate unread message to the left ([<]) and to the right ([>]) of the range.
  • Refactor tabs (divide status_bar_draw() with _status_bar_draw_tabs() function) and don't draw them if /statusbar maxtabs set to 0.

Revision 2

  • Remove excessive debug comments

@H3rnand3zzz H3rnand3zzz marked this pull request as ready for review November 8, 2023 10:05
@H3rnand3zzz
Copy link
Contributor Author

Revision 3

Improve commit message.

@H3rnand3zzz
Copy link
Contributor Author

Revision 4

Calculate range bounds only once and pass it as arguments to width calculating function to avoid excessive computations.

@jubalh jubalh self-requested a review November 10, 2023 21:13
src/ui/statusbar.c Outdated Show resolved Hide resolved
@H3rnand3zzz
Copy link
Contributor Author

Revision 5

Addressed the issues that were pointed out in the code review.

@H3rnand3zzz
Copy link
Contributor Author

Revision 6

Rebased on master

@sjaeckel sjaeckel self-requested a review November 15, 2023 08:55
Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. IIUC this changes the default behavior of the statusbar.
    TBH I don't think that we should do that w/o at least having an option to disable it and IMO this new style shouldn't be the new default, but an option that can be enabled.

  2. I didn't look yet in detail at the changes, but I've one question: Did you test whether the actlist mode also still works? I guess so, since it seems to be a completely different code path, but I prefer to be on the safe side.

@H3rnand3zzz
Copy link
Contributor Author

H3rnand3zzz commented Nov 15, 2023

  1. IIUC this changes the default behavior of the statusbar.
    TBH I don't think that we should do that w/o at least having an option to disable it and IMO this new style shouldn't be the new default, but an option that can be enabled.
  1. How is the change making it worse? Any objective reason why it shouldn't be the default except that it changes old clunky behaviour that doesn't even show current tab when you exceed 10 tabs (maxtabs maximum)?

It's basically a fix for 2 issues mentioned earlier, not even a new feature, but rather a development of older and underdeveloped feature.

  1. Yes, just tested it out, it works the same. (but as you already now, I am not trustworthy)

@jubalh
Copy link
Member

jubalh commented Nov 15, 2023

The description of this PR is quite confusing to me. I read it several times and still wasn't sure what it does and how it changes the current behaviour.
AFAIK we already have < and > when a message in a window to the left/right appears.

So I pulled the changes and tested what happens.
What I saw is that if I have maxtabs set to 2. Then in our old behaviour we always saw the first 2 windows no matter which one was the active one. With this PR it always displays the current window and the closest ones around it.

So I think for some people this can be helpful to know where they are.
I'm not sure whether it should be the default behaviour or should be configurable.

What I didn't understand:
When I set maxtabs to 2. And navigate to window 3. Then I see window 2 and 4 also in the tabs on the statusbar. Didn't I configure it to show max 2 tabs? Before this PR we only display 2 tabs.

This is the only change I noticed when trying the PR. Was there something else that I did miss?

@jubalh
Copy link
Member

jubalh commented Nov 15, 2023

Quite often your PRs/issues contain a huge wall of text but somehow fail at bringing the information across to the reader.

If I understand your PR now correctly, then it should have been quite easy to understand it with a better description.
Something along the lines:

This PR changes the current behaviour which displays always the first tabs until `maxtabs` to display the X tabs around the currently selected tab. So if we are having a maxtab of 1 and the actively selected window is 2. Then 2 is displayed only.

So far we only displayed `>` to indicate there are more windows. Since this PR modifies the tabs that are displayed we also have an `<` indicator now for the indicating windows to the left side of the currently selected ones.

It's quite late and I'm tired so the words might not be perfect. But I guess you get what I mean with a better explanation.

You instead say:

    Indicate unread message to the left ([<]) and to the right ([>]) of the range.

And I thought: ok, dont we do that already? Or whats the current behaviour? Should I change my whole settings to understand which options are available and check how it differs?

And:

navigate through pages easily

Where I dont understand what it means at all. I can always navigate through the open windows easily...

So now that I understood what actually changes: I don't think this should just be the new default behaviour without any notice of it anywhere. No notice in the commit message or PR that the default behaviour changes and how...

The proposed change itself is fine. But right now I believe that there should be a setting to use this behaviour or our current default. Then there is actlist too etc. So there will be 3 ways. There needs to be a well thought through setting with explanation for it.

@H3rnand3zzz
Copy link
Contributor Author

The proposed change itself is fine. But right now I believe that there should be a setting to use this behaviour or our current default. Then there is actlist too etc. So there will be 3 ways. There needs to be a well thought through setting with explanation for it.

There is no objective reason to keep old behaviour. Current PR fixes 2+ issues, it's not a feature, I reclassify it as a "fix".

@H3rnand3zzz H3rnand3zzz changed the title Add dynamic paging to statusbar Fix paging in statusbar Nov 16, 2023
@jubalh
Copy link
Member

jubalh commented Nov 16, 2023

There is no objective reason to keep old behaviour. Current PR fixes 2+ issues, it's not a feature, I reclassify it as a "fix".

One reason could be that I open my windows in a certain order. Could be the 5 first windows are the ones that are most important to me. I always want to see the tabs for those windows. I immediately want to see when somebody writes me on those windows. The rest might be windows that I don't care much about. In this case the > would also be enough. Since it would just signal: and I have more messages in (for me) unimportant windows. That I cycle through with alt+a when I have extra free time. Your PR would break this workflow.

Another reason could be that I don't like the UI to change all the time. I don't want that the titlebar/statusbar changes when I change the focus of a window. Maybe I only want that the content of the window changes and the rest should look stable.

So there are 2 objective reasons. If I think harder I could probably find more.

Just because something isn't your use case or to your liking, doesn't mean it doesn't exist.

@H3rnand3zzz
Copy link
Contributor Author

static tabmode has been added as requested.

@jubalh
Copy link
Member

jubalh commented Nov 17, 2023

Much better. But now you did the opposite of:

IIUC this changes the default behavior of the statusbar.
TBH I don't think that we should do that w/o at least having an option to disable it and IMO this new style shouldn't be > the new default, but an option that can be enabled.

And the text is misleading again. Since you write static mode has been added. But compared to master it is actually your dynamic mode that has been added. So I would change the second commit so that static is still the default. And the new option is called dynamic.

Like this the behaviour will stay the same for users but they can choose to change to the dynamic or actlist modes.

Copy link
Member

@jubalh jubalh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change 2nd commit to actually preserve the default behaviour as default and add an option for your new mode with the name dynamic.

@H3rnand3zzz
Copy link
Contributor Author

H3rnand3zzz commented Nov 17, 2023

Poll

Old style to the left, new style to the right. Upvote for a new style as a new default, downvote for old style. Don't mind the margin as it's not a subject to change, pay attention only to the pages being displayed (ease of navigation). 8 windows are open, /statusbar maxtabs 5.

Win 1

image

Win 3

image

Win 4

image

Win 5

image

Win 7

image

Win 8

image

@jubalh jubalh changed the title Fix paging in statusbar Add new mode for statusbar tab behaviour Nov 17, 2023
H3rnand3zzz added a commit to H3rnand3zzz/profanity that referenced this pull request Nov 20, 2023
Separate it on `dynamic` mode, as opposed to `static` (ex `default`).

Despite usefulness of the solution,
it was not approved to be a new default.

Vote (link below) amongst users has shown inconclusive result
and it was not representative, as it had low number of participants.
profanity-im#1912 (comment)
@H3rnand3zzz
Copy link
Contributor Author

H3rnand3zzz commented Nov 20, 2023

New revision

Changed commit. Currently it makes a good history (first introduced a change that fixes the issues, then introduce new change that makes new behaviour non-default), commits contain sufficient explanations for the changes.
default is renamed to static, as it makes more sense based on the current changes.

Finalizing vote

image
Vote has shown inconclusive result, hence the best course of action would be ignoring it since it does not represent interests of users (just profanity's MUC contains hundreds of users, while try above has only 5 votes)

src/command/cmd_defs.c Outdated Show resolved Hide resolved
src/config/preferences.c Outdated Show resolved Hide resolved
Copy link
Member

@jubalh jubalh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

H3rnand3zzz added a commit to H3rnand3zzz/profanity that referenced this pull request Nov 20, 2023
Return static tabmode as default,
separate previous change in `dynamic` mode.

Despite usefulness of the solution,
it was not approved to be a new default.

Vote (link below) amongst users to change default
has shown inconclusive result and it was not representative,
as it had low number of participants.
profanity-im#1912 (comment)
@H3rnand3zzz
Copy link
Contributor Author

H3rnand3zzz commented Nov 20, 2023

New revision

  • static->default
  • commit message updated
  • command description updated

H3rnand3zzz added a commit to H3rnand3zzz/profanity that referenced this pull request Nov 20, 2023
Return static tabmode as default,
separate previous change in `dynamic` mode.

Despite usefulness of the solution,
it was not approved to be a new default.

Vote (link below) amongst users to change default
has shown inconclusive result and it was not representative,
as it had low number of participants.
profanity-im#1912 (comment)
@H3rnand3zzz
Copy link
Contributor Author

H3rnand3zzz commented Nov 20, 2023

New revisions

  • Fix typo paging->pagination (also in commit message)

This commit changes the current behaviour which displays always the first
tabs until `maxtabs` to display N tabs around the currently selected tab.

So if we are having a maxtab of 1 and the actively selected window is 2,
then 2 is displayed only.

So far we have only displayed `>` to indicate that there are more windows.
Since this PR shifts the range of tabs that are displayed we also add `<`
indicator now to indicate windows to the left of the currently displayed.

Fix profanity-im#1283
Fix profanity-im#1764
Return static tabmode as default,
separate previous change in `dynamic` mode.

Despite usefulness of the solution,
it was not approved to be a new default.

Vote (link below) amongst users to change default
has shown inconclusive result and it was not representative,
as it had low number of participants.
profanity-im#1912 (comment)
@jubalh
Copy link
Member

jubalh commented Nov 20, 2023

Thanks for the changes and for the feature!

@jubalh jubalh merged commit dc8776e into profanity-im:master Nov 20, 2023
6 checks passed
H3rnand3zzz added a commit to H3rnand3zzz/profanity that referenced this pull request Dec 28, 2023
Return static tabmode as default,
separate previous change in `dynamic` mode.

Despite usefulness of the solution,
it was not approved to be a new default.

Vote (link below) amongst users to change default
has shown inconclusive result and it was not representative,
as it had low number of participants.
profanity-im#1912 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Statusbar - not displaying as configured Always show focused window number in statusbar
3 participants