RFC: Add OnHighlighted callback to GridWrap #5961
Conversation
This allows to have diferent actions for when an item is selected or highlighted.
If you're happy with what I did here I will be happy to do the same for the mentioned widgets |
|
Keeping with semantic API naming, this callback should be named |
|
I'm curious what @andydotxyz thinks about this one |
|
Sorry @dweymouth but I think that using the Focus term here would be confusing - as according to the focus manager it is the Grid/tree/list that has focus. This highlight is just a hover indicator. i.e. if you tab around this item will not move and so having Additionally, in using-a-mouse-mode, we do not change focus when the mouse hovers items, which is what this code is adding a hook for. |
I may be misunderstanding this - but if the Grid was focusable AND the GridItem was focusable AND there was an Entry inside each item you have a nightmare of focusing. And if each item inside the grid is a label then being focusable makes no sense. Unless I have missed something it makes sense that the developer would add focusable child items if that is what they desire. |
Perhaps if we return to the simple definition of focusable as "I want to interact with this widget" we can see that a hover effect, or a simple container of child items, should not be handled by the focus manager. |
|
I'm way out of my dept here, and may not explain myself very well. but if that matters, in my case each item in the grid is a stack. I tried making a custom focusable widget and it was even more confusing as I had to handle way too many things just to achieve this. I do agree that Focus Vs Highlight / hoover can be quite confusing as me and @andydotxyz spent quite some time chatting back and forth until we both could understand each other. Either way ... if this is to be implemented let me know what is it that I need to do, and i'll happily do it within my ability |
widget/gridwrap.go
Outdated
| // in the GridWrap has been unselected. | ||
| OnUnselected func(id GridWrapItemID) `json:"-"` | ||
|
|
||
| // OnHighlited is a callback to be notified when a given item |
There was a problem hiding this comment.
will fix it.
It might also be useful to change currentFocus to currentHighlight if it helps clear up some nomenclature.
wouldn't it be a breaking change?
Should the current focus have a deprecated comment? or being it a private field its fine with just a s/currentFocus/currentHighlight/g ?
// Deprecated: use currentHighlight instead
currentFocus ListItemID
There was a problem hiding this comment.
Changing the name of a field that is unexported is not a breaking change.
There was a problem hiding this comment.
Great!!!
Then I went ahead and:
- Fixed the typo
- Renamed onFocused to onHighlighted on all collection widgets
- Added OnHighlighted callback to all collection widgets
- Added similar callbacks for the mouse in event to all collection widgets
As for the mouse events, I opted by having different callbacks for both keyboard and Mouse events, as this keeps them decoupled and gives options for users. Hope you are okay with it, otherwise please let me know what is it you prefer :)
output_video_compressed.mp4
OBS: The sample app I used in the video can be found on coolapso/fyne-collection-highlight-hover-demo
There was a problem hiding this comment.
I think I'd personally just have one callback to avoid potential confusion but I don't know what the other think
There was a problem hiding this comment.
Yes, the widgets APIs are based on behaviour not platform specific behaviours.
There is no difference to how OnSelected works for keyboard and mouse, so why would highlight be different?
There was a problem hiding this comment.
Alright ... removed the OnHovered() callback and Mouse hover events now use the OnHighlighted() callback.
Change currentFocys to currentHighlight to clear up naming confusion.
Add OnHighlighted callback Refactor currentFocus to currentHighlight
Add OnHighlighted callback Refactor currentFocus to currentHighlight
Add OnHighlighted callback Refactor currentFocus to currentHighlight
Add OnHovered callback to GridWrap Similar to OnHighlighted, but triggered by mouse hover.
Add OnHovered callback to List Similar to OnHighlighted, but triggered by mouse hover.
Add OnHovered callback to Table Similar to OnHighlighted, but triggered by mouse hover.
Add OnHovered callback to Tree Similar to OnHighlighted, but triggered by mouse hover.
|
Looked at some issues and this seems to fix / be related with: |
|
Is there anything I need to do regarding the failing test? doesn't seem related to anything I've done at looks like it has been failing for a while in other PRs 🤔 kinda looks like a problem related to the ubuntu runner? |
There is unfortunately a known race condition within the test driver. Don't worry about it. We will rerun until it is green. We ought to fix that one 😅 |
They are related but I don't think impact this PR. The former is looking at additional effects of the Table which is best added internally. The latter is an interesting question - but differentiating between Highlight and Focus will help the discussion I think. |
widget/table.go
Outdated
| } | ||
|
|
||
| if oldHover != nil && newHover != nil && *oldHover == *newHover { | ||
| return |
There was a problem hiding this comment.
Doesn't this return break hovering of headers?
There was a problem hiding this comment.
hmm, this basically makes it less spammy, making sure it only triggers the event if there is effectively a change in the hover, otherwise every time you move the mouse inside a cell it triggers the event.
I may be wrong, but If hovering over headers was supposed to be working, then it isn't. I've checked with the demo app, hovering over the headers does nothing, just like it does nothing with the current code, same by moving around the table with the direction keys it never goes over the headers.
Fyne demo app latest
2025-10-05_23-10-1759700377.mp4
My little demo/testing app using my fork:
2025-10-05_23-10-1759700471.mp4
There was a problem hiding this comment.
Yes I appreciate that it makes it less spammy - which is good.
But it also removes the ability for the headers to indicate hover.
They currently don't visibly show the highlight state, but it is tracked so we can do it - this change removes that so it is not possible.
You can see that with a change along these lines:
diff --git a/widget/table.go b/widget/table.go
index 835ea7280..360e800f1 100644
--- a/widget/table.go
+++ b/widget/table.go
@@ -1,6 +1,7 @@
package widget
import (
+ "log"
"math"
"strconv"
@@ -688,6 +689,7 @@ func (t *Table) hoverAt(pos fyne.Position) {
col := t.columnAt(pos)
row := t.rowAt(pos)
t.hoveredCell = &TableCellID{row, col}
+ log.Println("Hover", t.hoveredCell)
overHeaderRow := t.ShowHeaderRow && pos.Y < t.headerSize.Height
overHeaderCol := t.ShowHeaderColumn && pos.X < t.headerSize.Width
if overHeaderRow && !overHeaderCol {You'll see the negative cell values (indicating a header).
There was a problem hiding this comment.
Aw alright my bad then. should I try to make it less spammy for both headers and cells? or you prefer to keep it as is ?
There was a problem hiding this comment.
I think you were on the right path to reduce the spammy (we should not call a callback or function if it has no change :) ) - but preserving the -ve values for headers to work. Shouldn't be a big change from what you have now.
There was a problem hiding this comment.
Alright, guess this should do it!, the callback and the check for changes is only done at the end now!
There was a problem hiding this comment.
It looks like some of the confusion about Hover vs Highlight is that the widgets have not all implemented correctly so that it's clearly the same thing.
They were probably coded at separate times but I don't think that there should be separate indicators depending on whether you are keyboard or mouse. Perhaps there is more to reconcile so it is as unified as it should be?
What do you mean? 🤔 |
As noted on the issue you opened it seems like hover and highlight have been coded separately and just happen to look the same. Realistically they should be the same as @Jacalz noted above regarding the single callback |
hmm yeah understand .. but guess that should probably be outside of the scope of this pr and refactored in a separate one? 🤔 |
Yes the refactor can be separate - as long as the public facing API and behaviour is as intended. |
|
Please don't force push after a review has started. GitHub has a tendency to destroy the history of review comments when commit ids no longer match. We can squash on merge instead of we feel that it is needed. |
🤦 never seen that happen, but I can totally see that being a thing! will do that. |
|
is this test failing because of my changes or is it also race condition on the test suite here to? |
When widgets are focueed they highlight a item, and this was not triggering the OnHighlighted event.
|
If you can merge in develop then the races in test should go away. Also please try to avoid force-pushing once the reviews have started because it means we have to start from scratch each time. |
alright done! I've merged upstream instead of rebasing, should do the trick. |
5 participants
Description:
when using a GridWrap widget, sometimes may be useful do do actions when the Item is Highlighted (focused?) without having to select it!
GridWrapcontains the private fieldcurrentFocuswhich represents the currently highlighted items in the grid however to get the ID of the highlighted item its non trivial and needs to be done with reflection as it is not exposed in the public api.This PR aims to expose the
currentFocusthrough the OnHighlighted callback thus allowing to have different actions for when an item is highlighted or effectively selected. Hoever I'm exposing it only through Keyboard as this might be undesirable when using the mouse.2025-10-03_23-10-1759525812.mp4
I might need some hands holding and opinions about this, and what I may have missed. E.g: Tests.
its okay if this actually goes against fyne opinions and should not be implemented but it was well worth the learning time learning it!
Checklist:
Where applicable: