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 color picker dismissed callback #5241

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

Conversation

ole108
Copy link

@ole108 ole108 commented Nov 5, 2024

Description:

Call the callback provided to the color picker dialog with a nil color if the dialog is dismissed.
So users can easily distinguish between confirmation and dismissal and react to it.
This is inline with how all the file related dialogs work (fileSave, fileOpen and folderOpen).

Fixes #5237

Checklist:

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

Where applicable:

@andydotxyz
Copy link
Member

I would prefer that we match the data input dialogs not the file dialogs.
I thought the bug you reported was that the close and callback were not called in the right order?

@ole108
Copy link
Author

ole108 commented Nov 6, 2024

I didn't look at the data input dialogs at all yet.
That's next.
Other order of close and callback would fix it, too.
I can dig into that. I suggest that both would be best.

@andydotxyz
Copy link
Member

The trouble with adding a "callback(nil)" to all dialogs is that some are picking an item that has a valid zero value.
i.e. Entry with a string may allow empty string which should not be accidentally confused with cancel.

The File dialog "calls nil on cancel" was added before the SetOnClosed was added. Maybe it shouldn't be there at all?

@ole108
Copy link
Author

ole108 commented Nov 6, 2024

OK. I will look into the order of callbacks tomorrow.
That would work for me, too.
And consistency would be great.
I might have started my Fyne journey at the wrong end. 🤷

@ole108
Copy link
Author

ole108 commented Nov 7, 2024

I updated the PR with the correct callback order.
I hope this is good now.

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 change of order seems reasonable, but the adding of possible panics worries me.

dialog/color.go Outdated Show resolved Hide resolved
@andydotxyz
Copy link
Member

I have looked into this to approve and only just realised (sorry if there was a discussion before) that this makes the close order different for the colour picker than for all others.
I agree that the change makes sense, but probably it needs to apply to dialog/base.go:101 as well so that dialogs that use the builtin callback get data before dismissal.

It could be checked by using the other data input dialogs (Form, Entry) in the context of your dialog library to see if you get data from either of those.

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.

2 participants