-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 chat console scrollbar #15104
Add chat console scrollbar #15104
Conversation
Why does this need a setting? |
The console window is a feature pretty much EVERY player interacts with, and it hasn't really changed in years, so I thought one should have the option not to use such a newly added feature. |
42d48af
to
3b3b1c7
Compare
5720885
to
73695ee
Compare
I would only add a setting if there is a solid user need for it. Each setting we add increases complexity of testing and has a cost when navigating the settings menu |
2df8591
to
cfbafa9
Compare
You say this PR closes #14994 and "Mobile players gain the ability to scroll in chat", but you can't even open the chat console on Android currently (except by attaching a physical keyboard), so how does this PR help Android players? |
Is this something my PR broke?
The idea was that it allows mobile players to scroll up using the scrollbar instead of the non-present mouse wheel. |
f1dcc62
to
71dcc8a
Compare
Unfortunately, not quite - grorp is pointing out that this PR only does part of the work required to solve #14994, since this feature is inaccessible to most Android users. This does not imply that this PR isn't good to merge as-is (though it would of course be ideal to have the "full package") or that it isn't a step in the right direction, but it's important for us to know so that we don't (automatically) close the issue by merging this PR if it isn't fully resolved. |
Sorry for the late reply, however, I think I'd rather have this PR only add the scrollbar as I am unfamiliar with compiling & debugging minetest for android. |
6d6113e
to
dd228df
Compare
@Zughy : The PR is ready to me reviewed once again, may you remove "Rebase needed" again please? |
This PR currently causes a malfunction in the page up/down scrolling. Now the chat only scrolls by a few lines for each key press event. EDIT: Correction. This is only an issue as long the scrollbar is focused. Hence this might be a deeper issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works.
bdecc10
to
6ae2a25
Compare
6ae2a25
to
5c59851
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usable, works as expected on desktop. I took the liberty to rebase & squash for my testing convenience. I've also added a tiny commit to use irr_ptr
for the memory management (5c59851), so scream if you're unhappy with that :P
I think I'd rather have this PR only add the scrollbar as I am unfamiliar with compiling & debugging minetest for android.
In case you want to work on a follow-up, maybe this helps with compiling: https://dev.luanti.org/developing-luanti-for-android/
This pull request adds a scrollbar to the console window.
Works towards closing #14994
If not a bug fix, why is this PR needed? What usecases does it solve?
To do
This PR is ready for review.
How to test
Don't forget the "big console window", which can be opened by pressing F10