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 chat console scrollbar #15104

Merged
merged 2 commits into from
Jan 12, 2025
Merged

Conversation

chmodsayshello
Copy link
Contributor

@chmodsayshello chmodsayshello commented Sep 1, 2024

This pull request adds a scrollbar to the console window.
image

Works towards closing #14994

If not a bug fix, why is this PR needed? What usecases does it solve?

  1. Mobile players gain the ability to scroll in chat
  2. Serves as an indicator whilst scrolling

To do

This PR is ready for review.

  • Keep scroll up-to-date while console is opened

How to test

  • Clone and compile my branch like usual
  • Open a game
  • Fill your console/chat with text (spamming)
  • Verify that upon a scrollbar appears upon it "overflowing"
  • Verify that the scrollbar can indeed be used to scroll

Don't forget the "big console window", which can be opened by pressing F10

@rubenwardy
Copy link
Contributor

Why does this need a setting?

@chmodsayshello
Copy link
Contributor Author

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.
But IF i keep the setting, I'll at least set it to true by default regardless of the platform.

@chmodsayshello chmodsayshello changed the title WIP: Add (optional) console-scrollbar Add (optional) console-scrollbar Sep 1, 2024
@Zughy Zughy added Feature ✨ PRs that add or enhance a feature UI/UX @ Client / Controls / Input labels Sep 1, 2024
@rubenwardy
Copy link
Contributor

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. But IF i keep the setting, I'll at least set it to true by default regardless of the platform.

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

src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
@chmodsayshello chmodsayshello requested a review from grorp September 4, 2024 15:55
@chmodsayshello chmodsayshello changed the title Add (optional) console-scrollbar Add console-scrollbar Sep 4, 2024
src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
src/gui/guiChatConsole.h Outdated Show resolved Hide resolved
@Zughy Zughy added the Roadmap The change matches an item on the current roadmap label Sep 8, 2024
@grorp
Copy link
Member

grorp commented Sep 11, 2024

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?

@chmodsayshello
Copy link
Contributor Author

chmodsayshello commented Sep 13, 2024

but you can't even open the chat console on Android currently (except by attaching a physical keyboard)

Is this something my PR broke?

how does this PR help Android players?

The idea was that it allows mobile players to scroll up using the scrollbar instead of the non-present mouse wheel.
I did not test on android, I only mentioned it as this pull request implements exactly the feature described in #14994

@appgurueu
Copy link
Contributor

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. I did not test on android, I only mentioned it as this pull request implements exactly the feature described in #14994

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.

@chmodsayshello
Copy link
Contributor Author

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.
As you have already altered the description of the pull request to say "Works towards closing" instead, I'll just keep resolving merge conflicts from now on whenever they occur and wait for reviews.

@Zughy Zughy added the Rebase needed The PR needs to be rebased by its author label Oct 13, 2024
@chmodsayshello
Copy link
Contributor Author

@Zughy : The PR is ready to me reviewed once again, may you remove "Rebase needed" again please?

@Ekdohibs Ekdohibs removed the Rebase needed The PR needs to be rebased by its author label Oct 13, 2024
src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
src/gui/guiChatConsole.cpp Show resolved Hide resolved
src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
src/gui/guiChatConsole.cpp Outdated Show resolved Hide resolved
@SmallJoker
Copy link
Member

SmallJoker commented Dec 30, 2024

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.

@SmallJoker SmallJoker added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Dec 30, 2024
@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 3, 2025
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Works.

@sfan5 sfan5 changed the title Add console-scrollbar Add chat console scrollbar Jan 6, 2025
Copy link
Contributor

@appgurueu appgurueu left a 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/

@sfan5 sfan5 merged commit d4a6df3 into luanti-org:master Jan 12, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Controls / Input Feature ✨ PRs that add or enhance a feature Roadmap The change matches an item on the current roadmap >= Two approvals ✅ ✅ UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants