Have chats respond to PgUp and PgDown #4158
Conversation
WalkthroughAdds a PageScrollHandler utility to provide PAGE_UP / PAGE_DOWN page-wise scrolling for JavaFX chat ListView; integrates it into chat list view, list controller, chat container controller, and view to route and consume page-key events in the chat lifecycle. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatView as ChatMessageContainerView
participant Controller as ChatMessageContainerController
participant ListCtrl as ChatMessagesListController
participant ListView as ChatMessagesListView
participant Handler as PageScrollHandler
participant ScrollBar
User->>ChatView: Press PAGE_UP / PAGE_DOWN
ChatView->>Controller: onPageUpKeyPressed()/onPageDownKeyPressed()
Controller->>ListCtrl: scrollPageUp()/scrollPageDown()
ListCtrl->>ListView: delegate to view's PageScrollHandler
ListView->>Handler: scrollPageUp()/scrollPageDown()
Handler->>ScrollBar: adjust value by blockIncrement
ScrollBar-->>Handler: updated value
Handler-->>ListView: return handled boolean
ListView-->>Controller: handled boolean
Controller-->>ChatView: handled boolean (consume event if true)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/desktop/src/main/java/bisq/desktop/common/utils/PageScrollHandler.java (1)
99-104: Consider refining event consumption logic (optional).The adjustScrollBar method always returns true, causing the key event to be consumed even when the ScrollBar is already at its limit and cannot scroll further. For the chat list use case this is probably acceptable to prevent event bubbling. However, if you want more precise control, consider returning true only when the value actually changes:
private boolean adjustScrollBar(ScrollBar vbar, double increment) { double oldValue = vbar.getValue(); vbar.adjustValue(oldValue + increment); - log.debug("ScrollBar adjusted: {}; {} -> {}", vbar, oldValue, vbar.getValue()); - return true; + double newValue = vbar.getValue(); + log.debug("ScrollBar adjusted: {}; {} -> {}", vbar, oldValue, newValue); + return oldValue != newValue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/desktop/src/main/java/bisq/desktop/common/utils/PageScrollHandler.java(1 hunks)apps/desktop/desktop/src/main/java/bisq/desktop/main/content/chat/message_container/list/ChatMessagesListView.java(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-20T14:44:55.236Z
Learnt from: HenrikJannsen
Repo: bisq-network/bisq2 PR: 3402
File: apps/desktop/desktop/src/main/java/bisq/desktop/main/content/chat/message_container/list/ChatMessagesListController.java:839-867
Timestamp: 2025-04-20T14:44:55.236Z
Learning: In ChatMessagesListController, offerbook messages from banned or ignored users are filtered at the creation of the list items through the bisqEasyOfferbookMessageService.isValid() method rather than in the getPredicate() method which filters only non-offerbook messages.
Applied to files:
apps/desktop/desktop/src/main/java/bisq/desktop/main/content/chat/message_container/list/ChatMessagesListView.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Build network module on windows-latest / Build network module on windows-latest
- GitHub Check: Build Bisq on ubuntu-latest / Build Bisq on ubuntu-latest
- GitHub Check: Build Bisq on macOS-latest / Build Bisq on macOS-latest
- GitHub Check: Build network module on ubuntu-latest / Build network module on ubuntu-latest
- GitHub Check: Build tor module on ubuntu-latest / Build tor module on ubuntu-latest
- GitHub Check: Build network module on macOS-latest / Build network module on macOS-latest
- GitHub Check: Build Bisq on windows-latest / Build Bisq on windows-latest
- GitHub Check: Build tor module on macOS-latest / Build tor module on macOS-latest
- GitHub Check: Build tor module on windows-latest / Build tor module on windows-latest
- GitHub Check: Build apps module on ubuntu-latest / Build apps module on ubuntu-latest
- GitHub Check: Build apps module on windows-latest / Build apps module on windows-latest
- GitHub Check: Build apps module on macOS-latest / Build apps module on macOS-latest
🔇 Additional comments (4)
apps/desktop/desktop/src/main/java/bisq/desktop/main/content/chat/message_container/list/ChatMessagesListView.java (1)
23-23: LGTM! Clean integration of PageScrollHandler.The PageScrollHandler integration follows the established pattern in this class: field declaration, initialization in constructor, subscribe on view attach, and unsubscribe on view detach. The lifecycle management matches the pattern used for other subscriptions (hasUnreadMessagesPin, showScrolledDownButtonPin).
Also applies to: 78-78, 98-98, 178-178, 193-193
apps/desktop/desktop/src/main/java/bisq/desktop/common/utils/PageScrollHandler.java (3)
1-41: LGTM! Well-designed utility class.The class documentation is clear and comprehensive. The implementation properly encapsulates the key event handling logic, implements the Subscription interface for lifecycle management, and uses defensive programming with Objects.requireNonNull.
60-77: LGTM! Event handling and subscription lifecycle are well implemented.The event handler correctly identifies PAGE_UP and PAGE_DOWN keys, performs the scroll action, and consumes the event only when scrolling occurs. The defensive unsubscribe() call in subscribe() prevents duplicate handler registration.
43-46: Verify the "VirtualScrollBar" CSS selector syntax in the lookupAll() call.In standard JavaFX CSS, style class selectors require a dot prefix (e.g.,
.scroll-bar). The selector"VirtualScrollBar"without a dot may not work as intended iflookupAll()is being used. Verify that this selector actually matches nodes or consider aligning with the fallback.scroll-barpattern, which should work for all ScrollBar instances.
neminatha
force-pushed
the
Have-chats-respond-to-PgUp-and-PgDown
branch
from
7815e79 to
5da2299
Compare
|
@axpoems Can you review? |
neminatha
force-pushed
the
Have-chats-respond-to-PgUp-and-PgDown
branch
3 times, most recently
from
7adecd3 to
d49c0f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/desktop/src/main/java/bisq/desktop/common/utils/PageScrollHandler.java(1 hunks)apps/desktop/desktop/src/main/java/bisq/desktop/main/content/chat/message_container/list/ChatMessagesListView.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/desktop/src/main/java/bisq/desktop/main/content/chat/message_container/list/ChatMessagesListView.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T08:41:55.679Z
Learnt from: rodvar
Repo: bisq-network/bisq2 PR: 4184
File: common/src/main/java/bisq/common/file/FileReaderUtils.java:46-48
Timestamp: 2025-12-17T08:41:55.679Z
Learning: When addressing Android API compatibility issues (e.g., Files.readString(), Locale.of()) in the bisq2 repository, centralize fixes in dedicated utility classes (e.g., LocaleUtils, FileUtils) instead of inline replacements. This improves maintainability and consistency. Apply this pattern broadly across Java source files; create wrapper/util methods for APIs that differ across Android/Java versions, and refactor call sites to use these utilities.
Applied to files:
apps/desktop/desktop/src/main/java/bisq/desktop/common/utils/PageScrollHandler.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Build tor module on macOS-latest / Build tor module on macOS-latest
- GitHub Check: Build apps module on ubuntu-latest / Build apps module on ubuntu-latest
- GitHub Check: Build apps module on macOS-latest / Build apps module on macOS-latest
- GitHub Check: Build network module on windows-latest / Build network module on windows-latest
- GitHub Check: Build tor module on windows-latest / Build tor module on windows-latest
- GitHub Check: Build apps module on windows-latest / Build apps module on windows-latest
- GitHub Check: Build tor module on ubuntu-latest / Build tor module on ubuntu-latest
- GitHub Check: Build network module on ubuntu-latest / Build network module on ubuntu-latest
- GitHub Check: Build network module on macOS-latest / Build network module on macOS-latest
- GitHub Check: Build Bisq on ubuntu-latest / Build Bisq on ubuntu-latest
- GitHub Check: Build Bisq on windows-latest / Build Bisq on windows-latest
- GitHub Check: Build Bisq on macOS-latest / Build Bisq on macOS-latest
🔇 Additional comments (6)
apps/desktop/desktop/src/main/java/bisq/desktop/common/utils/PageScrollHandler.java (6)
1-31: LGTM! Clean imports and standard license header.The package structure and imports are appropriate for this JavaFX utility class.
32-58: LGTM! Well-designed class structure with excellent documentation.The JavaDoc clearly explains the purpose, usage pattern, and thread-safety considerations. The use of
Objects.requireNonNullensures defensive programming, and implementingSubscriptionprovides clean lifecycle management.
77-85: LGTM! Correct event consumption pattern.The handler correctly consumes the key event only when a scroll action is performed, allowing the event to propagate when the scrollbar is absent or at its limit. This is the right UX behavior.
87-94: LGTM! Proper lifecycle management.The defensive
unsubscribe()call insubscribe()prevents duplicate event handlers, and the overall pattern integrates correctly with theSubscriptioninterface. The integration inChatMessagesListViewproperly calls these methods inonViewAttached()/onViewDetached().
96-114: LGTM! Clean delegation and proper use of ScrollBar's blockIncrement.The protected visibility allows subclassing if needed, and calling
findScrollBar()on each key press (rather than caching) handles dynamic scenarios where the scrollbar might appear or disappear. The performance impact is negligible for user-triggered key events.
116-122: LGTM! Correct scroll adjustment implementation.The use of
ScrollBar.adjustValue()properly handles clamping to min/max bounds, and returning whether the value changed enables correct event consumption. The comparisonoldValue != newValueon line 121 is safe since both values come from the sameScrollBarinstance, avoiding floating-point precision issues.
apps/desktop/desktop/src/main/java/bisq/desktop/common/utils/PageScrollHandler.java
Outdated
Show resolved
Hide resolved
axpoems
left a comment
axpoems
left a comment
There was a problem hiding this comment.
Looks good! However, it does not seem to work when the cursor is in the text input area of the chat.
neminatha
force-pushed
the
Have-chats-respond-to-PgUp-and-PgDown
branch
from
d49c0f8 to
7c06b04
Compare
Thank you — this was addressed.
Please review. |
neminatha
force-pushed
the
Have-chats-respond-to-PgUp-and-PgDown
branch
2 times, most recently
from
ef77f14 to
465be26
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/desktop/src/main/java/bisq/desktop/common/utils/PageScrollHandler.java`:
- Around line 129-133: The adjustScrollBar(ScrollBar vbar, double increment)
method misuses ScrollBar.adjustValue(...) (which expects a fractional 0.0–1.0
position) and should instead compute a new absolute value = vbar.getValue() +
increment, clamp it between vbar.getMin() and vbar.getMax(), call
vbar.setValue(clampedValue), and return whether the value changed; update the
adjustScrollBar implementation to use vbar.getMin()/getMax() for clamping and
vbar.setValue(...) rather than adjustValue(...) so delta-based scrolling behaves
correctly.
apps/desktop/desktop/src/main/java/bisq/desktop/common/utils/PageScrollHandler.java
Outdated
Show resolved
Hide resolved
neminatha
force-pushed
the
Have-chats-respond-to-PgUp-and-PgDown
branch
from
465be26 to
0a8279b
Compare
3 participants
Description
Resolves #2087
Enables pagewise scrolling with PAGE_UP/PAGE_DOWN keys in chat views.
Changes
PageScrollHandlerto manage keyboard navigation in scrollable controlsChatMessagesListViewfor improved UXDetails
The
PageScrollHandlerinstalls a key event handler that detects PAGE_UP/PAGE_DOWN key presses and adjusts the vertical scrollbar by its block increment, mimicking a page click on the scrollbar trough. This provides a more intuitive and efficient navigation experience in chat message lists.Summary by CodeRabbit
New Features
Bug Fixes / UX
✏️ Tip: You can customize this high-level summary in your review settings.