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.

Sign up for GitHub

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

Jump to bottom

Fix scrolling #1935

Merged
merged 1 commit into from
Jan 8, 2024
Merged

Fix scrolling #1935

merged 1 commit into from
Jan 8, 2024

Conversation

H3rnand3zzz
Copy link
Contributor

@H3rnand3zzz H3rnand3zzz commented Nov 23, 2023 •
edited
Loading

Current state:

  • Fixes "overscrolling"
  • Lag reduction (only necessary lines are being drawn)

Currently it's full of debugging lines with some issues which are to be fixed before release:

  • Fix "underscrolling"
  • Fix "overscrolling" (profanity jumps over big messages received from DB)
  • Change line calculation (made based on curses' position)
  • Cleanup debug lines
  • Merge
  • Fix a problem: extremely big message (e.g. 3k lines) causes profanity to jump back once scrolled on top

This line is for github CI/CD system to link the issue

Fix #1934

@H3rnand3zzz
Copy link
Contributor Author

New revision

  • Git commit description is removed for now since a lot of things are subject to change.
  • New approach: let ncurses calculate lines instead of trying to do it by ourselves. Passing numbers of lines in the buffer helped to resolve overscrolling problem and can be handy in the future (jumping to some messages (unread, search), visual selection of messages etc.).
  • Underscrolling is broken again since some numbers are off in line calculation (though they are mostly correct, it's likely caused by some edge cases), requires further investigation.

@H3rnand3zzz H3rnand3zzz force-pushed the fix/broken-scroll branch 2 times, most recently from 6cd05b6 to 2e122e8 Compare December 4, 2023 13:01
@H3rnand3zzz
Copy link
Contributor Author

New revision

  • Pass id from DB (I am not sure if it's correct, probably stanza-id should be passed, but also we should consider LMC case)
  • Increase PAD_SIZE from 1000 to 10000, it helps rendering in the background, as well as with lines amount calculation
  • More debug lines (to be removed before staging)

@H3rnand3zzz H3rnand3zzz force-pushed the fix/broken-scroll branch 2 times, most recently from e4a33aa to afff1f1 Compare December 12, 2023 12:16
@H3rnand3zzz H3rnand3zzz changed the title Fix scrolling (work in progress, poc) Fix scrolling Dec 12, 2023
@H3rnand3zzz H3rnand3zzz marked this pull request as ready for review December 12, 2023 12:16
@H3rnand3zzz
Copy link
Contributor Author

New revision

  • Commit message that outlines changes
  • Merged with the latest changes
  • Added fetching stanza_id from the DB (changes are highly interconnected)
  • Removed debug messages

@jubalh jubalh requested review from sjaeckel and jubalh December 12, 2023 14:37
@H3rnand3zzz
Copy link
Contributor Author

H3rnand3zzz commented Dec 13, 2023 •
edited
Loading

New revision

  • Comments are removed as per @sjaeckel's request.
  • Variable renamed for more consistent style

@jubalh
Copy link
Member

jubalh commented Dec 14, 2023

Pass id from DB (I am not sure if it's correct, probably stanza-id should be passed, but also we should consider LMC case)

What were your thoughts here exactly? I think id that you used is correct.

@jubalh jubalh added this to the next milestone Dec 14, 2023
@H3rnand3zzz
Copy link
Contributor Author

New revision

Addressed the issues outlined in the review.

@H3rnand3zzz
Copy link
Contributor Author

addressed

@H3rnand3zzz H3rnand3zzz marked this pull request as draft December 15, 2023 16:21
@jubalh
Copy link
Member

jubalh commented Dec 20, 2023

Fix a problem: extremely big message (e.g. 3k lines) causes profanity to jump back once scrolled on top

Was this already the case before your change?
I'm not even sure 3k lines fit on a screen? :D

@H3rnand3zzz H3rnand3zzz marked this pull request as ready for review December 21, 2023 14:24
@H3rnand3zzz
Copy link
Contributor Author

H3rnand3zzz commented Dec 21, 2023 •
edited
Loading

New revision

  • Removed buffer limitation based on amount of lines in messages: it might affect users who don't use history, the approach has a good idea, but requires more careful implementation (e.g. limitation on amount of drawn lines as opposed to stored lines)
  • Description updated
  • Rebased on master

@jubalh
Copy link
Member

jubalh commented Dec 21, 2023

it might affect users who don't use history, the approach has a good idea, but requires more careful implementation

Affect how? Should we wait with merging/reviewing the code before this is solved? I would prefer to not include something "wrong" in master because several of our users use master to help us find bugs. So this might annoy them or make them report something that we are aware of.

@H3rnand3zzz
Copy link
Contributor Author

it might affect users who don't use history, the approach has a good idea, but requires more careful implementation

Affect how? Should we wait with merging/reviewing the code before this is solved? I would prefer to not include something "wrong" in master because several of our users use master to help us find bugs. So this might annoy them or make them report something that we are aware of.

It might've affected in a previous version, currently it was removed, hence no one should be affected in any negative way (at least I don't see a problem currently, I might've missed something obviously).

jubalh
jubalh reviewed Dec 21, 2023
View reviewed changes
src/database.c

ProfMessage* msg = message_init();
msg->id = id ? strdup(id) : NULL;
Copy link
Member

Choose a reason for hiding this comment

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

I see you introduced STRDUP_OR_NULL in buffer.c. I wonder if it would be worth it to have this in common.h and use it here (and in other places) as well? @sjaeckel ?

Copy link
Member

Choose a reason for hiding this comment

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

@sjaeckel ping

jubalh
jubalh reviewed Dec 21, 2023
View reviewed changes
src/ui/buffer.c Outdated Show resolved Hide resolved
@H3rnand3zzz
Fix overscrolling issue
4fe2c42 
Before there was a problem of overscrolling:
when messages longer than y axis of the terminal are fetched from the DB,
profanity scroll "jumps" to the top, skipping some messages.

It's resolved by keeping messages' starting and ending line in the
internal profanity buffer, which allows to track proper message positions
and to adjust window position accordingly.

Message size is now tracked as part of the buffer's record in `_line`
variable, which allows calculation of the total buffer size, which
might be a part of the improved solution for the "underscrolling" problem,
if we are going to limit profanity's buffer size by amount of lines as
opposed to the limitation based on the amount of message which is currently
used.

Before adding a limitation by amount of lines, careful consideration is
required, as some users don't use history and their temporary message
history can be cut to minimal limit because of 1 long received/sent message.

Underscrolling problem was fixed in a previous commit
d7e46d6
Short recap of the problem:
Despite user scrolling to top/bottom of history,
factual position is offset from the intended location

Another feature of this commit is a minor change which adds fetching
message stanza IDs from the DB. It allows correcting messages
fetched from history.

Fixes profanity-im#1934
@H3rnand3zzz
Copy link
Contributor Author

New revision

  • Fixed the issue pointed out above (buffer wasn't limited at all)

@jubalh jubalh merged commit 2ec9406 into profanity-im:master Jan 8, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jubalh jubalh jubalh left review comments

@sjaeckel sjaeckel Awaiting requested review from sjaeckel

Assignees

@H3rnand3zzz H3rnand3zzz

Labels
improvement
Projects
None yet
Milestone
next
Development

Successfully merging this pull request may close these issues.

Problem of under- and overscrolling
2 participants
@H3rnand3zzz @jubalh