Add .SetIcon() to custom dialogs #5386

lostdusty · 2025-01-09T19:44:33Z

Description:

I've wanted to include for some time a new API that allows the developer to set whatever resource for the text-based dialog(s) icon. This allows for an example, a success dialog, confirming the user changes.

Checklist:

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

Where applicable:

Public APIs match existing style and have Since: line.

Jacalz

We discussed new dialogs a long time ago with some v2 dialog stuff that I intended to work on but never got around to. The problem now is that a lot of details are internal (like your new dialogs just being exported functions that call a single internal one) and the dialog API is very old and fragile without much happening. It just doesn't scale well and feels unintuitive compared to more modern and developer friendly APIs that we have in other places.

I think we really should be very careful about adding more dialogs until we have a better API and can expose settings in a tidier way. Adding a new exported constructor API just to change a single internal parameter does not scale well.

Jacalz · 2025-01-09T20:12:47Z

A better API in the meantime would perhaps be to just allow setting the icon on some of the other dialogs? You can look at the button row changes that I did a while ago.

PS: It should be Since: 2.6.0 for the new APIs.

andydotxyz

I don't think this belongs in the info files - we have other source for custom dialogs don't we?

Jacalz

Yeah, I would personally just add a method similar to

fyne/dialog/custom.go

Line 53 in 4a875d9

func (d *CustomDialog) SetButtons(buttons []fyne.CanvasObject) {

on the custom dialog struct.

lostdusty · 2025-01-09T20:43:18Z

I have tried adding a SetIcon on custom but it did not work, at least not with NewCustomDialog

Jacalz · 2025-01-09T20:56:42Z

It should definitely be possible to do, or might require tweaking some minor stuff, but you might have been bitten by the strange semantics of how dialogs work. You basically have to recreate the dialog with a call to .create() to populate the internal layout and objects. I worked around this, or optimized it, with setButtons to avoid recreating the whole dialog.

lostdusty · 2025-01-09T23:29:17Z

I think I managed to do it

…alog

coveralls · 2025-01-09T23:44:14Z

coverage: 59.243% (+0.01%) from 59.232%
when pulling 44404db on lostdusty:patch-1
into b7b8a61 on fyne-io:develop.

Jacalz

Thanks. Looking good. Just a few suggestions inline

dialog/base.go

dialog/custom.go

Co-authored-by: Jacob Alzén <[email protected]>

Jacalz

Thanks. I noticed a small bug when setting the icon to nil, left a note inline.

I forgot to write this earlier but would you please add some tests? You can look at how the tests around the setButton functionality was done.

FYI: We usually work on the principle that the one adding the review comment is the one that decide when it is resolved (and marks it as such).

dialog/base.go

lostdusty · 2025-01-11T17:39:08Z

How do I make the test image? Should I use the window.Canvas().Capture() function?

lostdusty · 2025-01-11T18:10:57Z

Used it, all tests passed for me :)

Jacalz

Thanks. Well done with the tests. Really close now. Let's just combine the tests into one and use correct naming :)

Jacalz · 2025-01-11T19:50:09Z

dialog/custom_test.go

+}
+
+func TestCustomSetIconNil(t *testing.T) {
+	test.NewTempApp(t)
+	w := test.NewTempWindow(t, canvas.NewRectangle(color.Transparent))
+	size := fyne.NewSize(200, 300)
+	w.Resize(size)
+
+	test.ApplyTheme(t, test.Theme())
+	label := widget.NewLabel("Test was successful.")
+	d := NewCustom("Fyne test", "Dimiss", label, w)
+	d.SetIcon(nil)
+	d.Show()
+
+	test.AssertRendersToImage(t, "dialog-custom-seticon-nil.png", w.Canvas())


You don't need two different test functions just for setting different values. There is just unnecessary code duplication between them. I'd rather call the test TestCustom_SetIcon to be consistent with the other tests (that is the standard for naming) and then just at the end of the first one you have, set the icon to nil and do a new assert to image.

Basically keep the first test intact but change the name and then add .SetIcon(nil) and the asserting to image call. No more than two lines need to be added to the first test, I think.

andydotxyz · 2025-01-11T20:19:14Z

I am pretty sure the failures were resolved in more recent commits on develop branch, if you rebase or merge it in things should be looking pretty good.

lostdusty · 2025-01-11T21:07:11Z

Does it looks better now? I'll try rebasing now...

lostdusty · 2025-01-11T21:14:01Z

Hope its good now :)

Jacalz · 2025-01-11T21:39:24Z

Oh no. You are running into the horrible test failure that I encountered

Jacalz · 2025-01-11T21:40:14Z

It is a test that hard-codes the lookup of the amount of files in the folder where you put your images. Now there are 11 instead of 9 so it is failing everywhere

Jacalz · 2025-01-11T21:41:34Z

That test is really smelly. We need to do something about that:

fyne/dialog/file_test.go

Lines 420 to 450 in 4a875d9

    
           // NOTE: This count needs to be updated when more test images are added. 
        
           assert.Equal(t, 8, count) 
        
           f.SetFilter(storage.NewMimeTypeFileFilter([]string{"image/jpeg"})) 
        
           count = 0 
        
           for _, uri := range f.dialog.data { 
        
           	ok, _ := storage.CanList(uri) 
        
           	if !ok { 
        
           		assert.Equal(t, uri.MimeType(), "image/jpeg") 
        
           		count++ 
        
           	} 
        
           } 
        
           // NOTE: This count needs to be updated when more test images are added. 
        
           assert.Equal(t, 1, count) 
        
           f.SetFilter(storage.NewMimeTypeFileFilter([]string{"image/*"})) 
        
           count = 0 
        
           for _, uri := range f.dialog.data { 
        
           	ok, _ := storage.CanList(uri) 
        
           	if !ok { 
        
           		mimeType := strings.Split(uri.MimeType(), "/")[0] 
        
           		assert.Equal(t, mimeType, "image") 
        
           		count++ 
        
           	} 
        
           } 
        
           // NOTE: This count needs to be updated when more test images are added. 
        
           assert.Equal(t, 9, count)

lostdusty · 2025-01-11T22:05:01Z

Mind if I ask why its hard-coded?

Jacalz · 2025-01-11T22:08:16Z

I don't know. I suppose the one who wrote the test didn't think of the consequences of having it fail any time someone adds a new file to the folder but it doesn't matter much. We all make mistakes. I added those warning comments last time I ran into it. The fact that it is problematic remains :)

andydotxyz · 2025-01-11T22:39:31Z

At the same time it's a pretty easy fix. Files added, test fails, update number, tests pass.

lostdusty · 2025-01-11T23:11:20Z

I mean I thought it had a particular reason, I'll update the file :)

lostdusty · 2025-01-11T23:12:47Z

Just to make sure, I just need to change from 9 to 11, other numbers should be fine, right?

Jacalz · 2025-01-12T07:09:38Z

You can run the test locally to see what it says. It should print an expected and an actual number. Just edit that and you should see when it passes

lostdusty · 2025-01-12T13:10:21Z

Should be working now, had to update two numbers.

andydotxyz

Clean addition thanks.

Jacalz

Great addition indeed. Well done!

proposal: CustomMessage dialogs

9629d82

lostdusty mentioned this pull request Jan 9, 2025

Proposal: CustomMessage dialogs #5385

Closed

4 tasks

Jacalz reviewed Jan 9, 2025

View reviewed changes

andydotxyz reviewed Jan 9, 2025

View reviewed changes

Jacalz requested changes Jan 9, 2025

View reviewed changes

move dialog to custom, allowing to set a custom icon in any custom di…

683f0f7

…alog

Jacalz changed the title ~~Proposal: CustomMessage dialogs~~ Add .SetIcon() to custom dialogs Jan 10, 2025

Jacalz requested changes Jan 10, 2025

View reviewed changes

dialog/base.go Show resolved Hide resolved

dialog/custom.go Outdated Show resolved Hide resolved

lostdusty and others added 2 commits January 10, 2025 12:53

change docs

bc29efe

Co-authored-by: Jacob Alzén <[email protected]>

use a spacer instead of an empty image

0cb1e9d

Jacalz requested changes Jan 10, 2025

View reviewed changes

dialog/base.go Show resolved Hide resolved

also refresh if nil

8463d5a

add tests for seticon

eb0028a

Jacalz requested changes Jan 11, 2025

View reviewed changes

update seticon tests

79b0bd7

Merge branch 'develop' into patch-1

6704d0a

also update file tests

44404db

andydotxyz approved these changes Jan 12, 2025

View reviewed changes

Jacalz approved these changes Jan 12, 2025

View reviewed changes

Jacalz merged commit 4a30df3 into fyne-io:develop Jan 12, 2025
12 checks passed

lostdusty deleted the patch-1 branch January 12, 2025 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add .SetIcon() to custom dialogs #5386

Add .SetIcon() to custom dialogs #5386

lostdusty commented Jan 9, 2025 •

edited

Loading

Jacalz left a comment

Jacalz commented Jan 9, 2025

andydotxyz left a comment

Jacalz left a comment

lostdusty commented Jan 9, 2025

Jacalz commented Jan 9, 2025 •

edited

Loading

lostdusty commented Jan 9, 2025

coveralls commented Jan 9, 2025 •

edited

Loading

Jacalz left a comment

Jacalz left a comment

lostdusty commented Jan 11, 2025

lostdusty commented Jan 11, 2025

Jacalz left a comment

Jacalz Jan 11, 2025

Jacalz Jan 11, 2025 •

edited

Loading

andydotxyz commented Jan 11, 2025

lostdusty commented Jan 11, 2025

lostdusty commented Jan 11, 2025

Jacalz commented Jan 11, 2025

Jacalz commented Jan 11, 2025

Jacalz commented Jan 11, 2025 •

edited

Loading

lostdusty commented Jan 11, 2025

Jacalz commented Jan 11, 2025

andydotxyz commented Jan 11, 2025

lostdusty commented Jan 11, 2025

lostdusty commented Jan 11, 2025

Jacalz commented Jan 12, 2025

lostdusty commented Jan 12, 2025

andydotxyz left a comment

Jacalz left a comment

Add .SetIcon() to custom dialogs #5386

Add .SetIcon() to custom dialogs #5386

Conversation

lostdusty commented Jan 9, 2025 • edited Loading

Description:

Checklist:

Where applicable:

Jacalz left a comment

Choose a reason for hiding this comment

Jacalz commented Jan 9, 2025

andydotxyz left a comment

Choose a reason for hiding this comment

Jacalz left a comment

Choose a reason for hiding this comment

lostdusty commented Jan 9, 2025

Jacalz commented Jan 9, 2025 • edited Loading

lostdusty commented Jan 9, 2025

coveralls commented Jan 9, 2025 • edited Loading

Jacalz left a comment

Choose a reason for hiding this comment

Jacalz left a comment

Choose a reason for hiding this comment

lostdusty commented Jan 11, 2025

lostdusty commented Jan 11, 2025

Jacalz left a comment

Choose a reason for hiding this comment

Jacalz Jan 11, 2025

Choose a reason for hiding this comment

Jacalz Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

andydotxyz commented Jan 11, 2025

lostdusty commented Jan 11, 2025

lostdusty commented Jan 11, 2025

Jacalz commented Jan 11, 2025

Jacalz commented Jan 11, 2025

Jacalz commented Jan 11, 2025 • edited Loading

lostdusty commented Jan 11, 2025

Jacalz commented Jan 11, 2025

andydotxyz commented Jan 11, 2025

lostdusty commented Jan 11, 2025

lostdusty commented Jan 11, 2025

Jacalz commented Jan 12, 2025

lostdusty commented Jan 12, 2025

andydotxyz left a comment

Choose a reason for hiding this comment

Jacalz left a comment

Choose a reason for hiding this comment

lostdusty commented Jan 9, 2025 •

edited

Loading

Jacalz commented Jan 9, 2025 •

edited

Loading

coveralls commented Jan 9, 2025 •

edited

Loading

Jacalz Jan 11, 2025 •

edited

Loading

Jacalz commented Jan 11, 2025 •

edited

Loading