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

Fix clicking on empty part of widget.List not unfocusing #5314

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

coder-in-go
Copy link

@coder-in-go coder-in-go commented Dec 13, 2024

Description:

This PR fixes the issue where clicking on an empty part of the widget list does not unfocus other widgets.

Fixes #4770

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

I don't understand the driver internals well enough to comment on the approach in question but this does seem to fix the issue and the code looks good to me. Would you mind adding a test case for this?

@Jacalz Jacalz requested a review from andydotxyz December 13, 2024 12:48
@@ -130,6 +130,8 @@ func (l *List) RefreshItem(id ListItemID) {
return
}
l.BaseWidget.Refresh()

l.Unselect(id)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like an unrelated change? Why are the semantics of RefreshItem changing to that you can't use it to refresh a selected item?

Copy link
Author

Choose a reason for hiding this comment

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

  • When the entry is focused, the list item should be unfocused, to unfocus the list item we need this

Copy link
Member

Choose a reason for hiding this comment

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

But this code is not allowing an item to remain selected when it is refreshed - so scrolling will lose the selected status

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Sorry but this breaks driver event propagation as described in my feedback

internal/driver/glfw/window.go Outdated Show resolved Hide resolved
internal/driver/glfw/window.go Outdated Show resolved Hide resolved
@andydotxyz
Copy link
Member

andydotxyz commented Dec 13, 2024

I don't think a driver change is the right way to solve this.
The list itself gets tap events and it will know if the tap is over an item or over space...

@Jacalz
Copy link
Member

Jacalz commented Dec 17, 2024

A little note on PR naming: Please avoid Fixes issue XYZ: in the beginning. As very few of us actually know which problem a given number corresponds to, it is better to just keep that information in the Fixes #XYZ part of the description. In this case I would have labelled the PR something along the lines: "Fix clicking on empty part of widget.List not unfocusing".

@Jacalz Jacalz changed the title Fixes issue 4770: Clicking on empty part of widget.List does not unfo… Fix clicking on empty part of widget.List not unfocusing Dec 17, 2024
Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

The PR looks to be doing the opposite of what the code specifies, is this a mistake?

With a unit test we would be able to verify this corrected behaviour.

if l.focused {
l.FocusLost()
}
canvas.Focus(l)
Copy link
Member

Choose a reason for hiding this comment

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

the PR says it is unfocusing but this code shows focussing - is this correct?

Copy link
Author

@coder-in-go coder-in-go Dec 19, 2024

Choose a reason for hiding this comment

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

the PR says it is unfocusing but this code shows focussing - is this correct?

when the empty portion is clicked the widgets (Entry, list items) are getting unfocused --> the issue

This PR resolves this issue --> The widgets get unfocused when we click the empty portion

Copy link
Member

Choose a reason for hiding this comment

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

So we should do this by unfocusing surely?
Focusing something else fixes it as a side-effect, not by actually fixing the issue!

@@ -130,6 +130,8 @@ func (l *List) RefreshItem(id ListItemID) {
return
}
l.BaseWidget.Refresh()

l.Unselect(id)
Copy link
Member

Choose a reason for hiding this comment

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

But this code is not allowing an item to remain selected when it is refreshed - so scrolling will lose the selected status

@coveralls
Copy link

Coverage Status

coverage: 59.642% (-0.3%) from 59.954%
when pulling 3b9c451 on coder-in-go:bug_fixes/Bug-4770
into 5ee9979 on fyne-io:develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants