Refresh dialogs correctly, fixes #1866 #1931

fpabl0 · 2021-02-09T18:40:35Z

Description:

Fixes #1866

Checklist:

…ix fyne-io#1866

fpabl0 · 2021-02-09T18:54:50Z

I have not updated the test images, because I am not sure why those images were correct before:

Before:

After:

…en windows is created and resized, fixes fyne-io#1692 when fyne-io#1931 is landed

dialog/base.go

widget/popup.go

…flicker, remove useless comments

toaster

The change in the theme test images is expected due to the bugfix.
Nevertheless, I’d prefer to have a new test which checks the particular situation #1866 describes.
The test should:

render the shown dialogue with theme A and check the image
hide the dialogue
switch the theme
show the dialogue and check the image
hide the dialogue again
switch the theme back
show the dialogue and check the image

toaster · 2021-02-15T06:49:33Z

@andydotxyz:
After examining this, I found that the dialog rendering looks good by chance.
I want to refactor it (for 2.1) to create a render subtree that reflects the expected output better. Also, I’d like to separate dialog from its layout. Any objections?

fpabl0 · 2021-02-15T07:30:03Z

I’d prefer to have a new test which checks the particular situation #1866 describes.

Got it. I will do it.
After looking carefully, I think the popup refresh should occur in popup.Show and not in dialog.Show because it is more a problem of the widget.Popup than the dialog.

Basically it would mean to have this in dialog:

func (d *dialog) Show() {
	if !d.desiredSize.IsZero() {
		d.win.Resize(d.desiredSize)
	}
	d.applyTheme() // <<< instead of d.Refresh()
	d.win.Show()
}

And this in widget.Popup:

// Show this pop-up as overlay if not already shown.
func (p *PopUp) Show() {
	if !p.overlayShown {
		if p.Size().IsZero() {
			p.Resize(p.MinSize())
		}
		p.Canvas.Overlays().Add(p)
		p.overlayShown = true
	}
	p.Refresh() // <<< added
	p.BaseWidget.Show()
}

What do you think? @toaster

Also, I’d like to separate dialog from its layout.

I think this part was already done by @andydotxyz in one of the latest commits.

toaster · 2021-02-15T07:48:43Z

After looking carefully, I think the popup refresh should occur in popup.Show and not in dialog.Show because it is more a problem of the widget.Popup than the dialog.

[…]

What do you think? @toaster

I think this whole thing is more a problem of all outside tree widgets and theme changes. Currently, we don’t have a public interface to apply theme changes and Refresh is not necessarily recursive.
As a workaround™ (probably in place forever ;)), I suggest to call internal.app.ApplyThemeTo() within BaseWidget’s Show() method.
@andydotxyz WDYT?

andydotxyz · 2021-02-15T10:35:42Z

Currently, we don’t have a public interface to apply theme changes and Refresh is not necessarily recursive.

We removed that because it was confusing/duplication.
The framework should ensure that Refresh() is called on all widgets through ApplyThemeTo.
See the code in internal/app/theme.go when the settings change the theme should already be applied.

If the dialog is not in the overlay list then it probably should ensure the same state by using
ApplyThemeTo(overlay, window.Canvas()). I don't think this should be put in BaseWidget as it only applies to overlay layers.

andydotxyz · 2021-02-15T10:36:17Z

Also, I’d like to separate dialog from its layout.

I think this part was already done by @andydotxyz in one of the latest commits.

Yes this has already been done.

…nd updated dialog's tests

fpabl0 · 2021-02-16T07:04:56Z

dialog/base.go

@@ -260,7 +268,7 @@ func (l *dialogLayout) Layout(obj []fyne.CanvasObject, size fyne.Size) {
 	l.d.bg.Move(fyne.NewPos(0, 0))
 	l.d.bg.Resize(size)

-	btnMin := obj[3].MinSize().Max(obj[3].Size())
+	btnMin := obj[3].MinSize()


I have removed the part .Max(obj[3].Size()) because it makes the dialog button ignore the theme change (sizing variables). @andydotxyz do you know if there was a reason to have that part?
By removing it, a test image from the color picker changed, however it seems that before this change, that test image was wrong:
Left side is after this change and right side is before this change:

I don't recall - but this is the change 72cac28

Hmm I see, that change breaks dialog.NewProgress..., however:
btnMin := obj[3].MinSize().Max(obj[3].Size())
Isn't the above code breaking the principle that objects inside a Layout cannot manipulate manually their size neither their position? @andydotxyz

Also, I have seen that both dialog.NewProgress and dialog.NewProgressInfinite are deprecated and the message says:

// Deprecated: Create a new custom dialog with a widget.ProgressBarInfinite() inside.

So using dialog.NewCustom the widget would be inside dialog content and not as a button as it is done currently inside dialog.NewProgress.
However it is still possible to make the dialog.NewProgress looks like before, doing this:

c := container.NewVBox(widget.NewProgressBarInfinite()) d := dialog.NewCustom("Title", "", c, w) d.Resize(fyne.NewSize(200, d.MinSize().Height)) d.Show()

But NewCustom always render a dismiss button, I think there should be a way to tell dialog.NewCustom to not render that button if user does not want it.

I think you are describing widget.NewModalPopUp

Sorry, I didn't understand, what do you mean?

Sorry, what I was meaning is: isn't a dialog with no buttons a modal popup?
I.e. dialogs are dismissable using buttons, to do something that isn't you can show a modal popup.

Either way, for now perhaps the right thing to do is place a comment next to that line saying "// TODO remove once progress dialogs are gone" - if there is no sensible way to fix the issue with this PR.

Maybe the comment in the deprecation should be fixed.
Anyhow, as I am sure you agree, we can't break the feature even if it's deprecated.

dialogs are dismissable using buttons, to do something that isn't you can show a modal popup.

Oh I see, yes you are right.

for now perhaps the right thing to do is place a comment next to that line saying "// TODO remove once progress dialogs are gone"

By doing it, a wrong test image needs to be committed, is that ok?

if there is no sensible way to fix the issue with this PR.

We could update dialog.NewProgress with the above code, then we don't need to commit wrong test images, while ensuring the same behavior.

Maybe the comment in the deprecation should be fixed.

Yes, I think.

Anyhow, as I am sure you agree, we can't break the feature even if it's deprecated.

Completely, agreed.

dialog/base.go

…o respect theme and canvas size changes) methods, fix popup tests

toaster · 2021-02-17T07:18:45Z

If the dialog is not in the overlay list then it probably should ensure the same state by using
ApplyThemeTo(overlay, window.Canvas()). I don't think this should be put in BaseWidget as it only applies to overlay layers.

Yes, as long as a dev does not remove an object from the tree, keeps it and adds it again later on. However, this is quite unlikely.

dialog/base.go

andydotxyz · 2021-02-18T18:58:51Z

Yes, as long as a dev does not remove an object from the tree, keeps it and adds it again later on. However, this is quite unlikely.

If required we could add that if an element has been hidden for a while it should have Refresh() called?
If we start removing renderers from hidden widgets after a time this may even happen automatically.

…lines

fpabl0 · 2021-02-18T21:49:15Z

It seems that there is a flaky test related to entry widget:
https://github.com/fyne-io/fyne/pull/1931/checks?check_run_id=1930590506#step:5:101

…en theme change in widget.PopUp, added popup tests

fpabl0 · 2021-02-19T08:53:50Z

I have added a little layout fix (that I didn't notice until I wrote the popup tests) for popup that would benefit dialog too, please kindly check.

toaster

LGTM

andydotxyz · 2021-02-21T12:58:50Z

Can we just clarify please, is the size / layout of the deprecated progress dialog still broken with this proposed change?

fpabl0 · 2021-02-21T14:44:16Z

Yes, it is broken, if we re-write the progress dialog to:

c := container.NewVBox(widget.NewProgressBarInfinite())
d.setButtons(c)
d.Resize(fyne.NewSize(200, d.MinSize().Height))

then it works as before.

fpabl0 · 2021-02-21T21:34:29Z

Well, my assumption was wrong the above code does not fix broken progress dialogs, but this does:

bar := widget.NewProgressBarInfinite()
rect := canvas.NewRectangle(color.Transparent)
rect.SetMinSize(fyne.NewSize(200, 0))

d.setButtons(container.NewMax(rect, bar))

Is that fine? It is just a workaround to a deprecated feature, so I think it would be ok, what do you think? @andydotxyz

andydotxyz · 2021-02-22T18:49:48Z

Is that fine? It is just a workaround to a deprecated feature, so I think it would be ok, what do you think? @andydotxyz

Yes, like you say it's a workaround but the code is on the way out.

andydotxyz · 2021-02-22T18:52:06Z

Looks great - are you still happy @toaster ?

toaster · 2021-02-23T08:50:09Z

Looks great - are you still happy @toaster ?

Yes, I am :).

* fix modalPopUpRenderer refresh and refresh dialog when it is shown to fix #1866 * update themedBackgroundRenderer functions to respect fyne style guidelines Conflicts: dialog/base.go

fix modalPopUpRenderer refresh and refresh dialog when it is shown to f…

0afcea3

…ix fyne-io#1866

Jacalz changed the title ~~Fix #1866~~ Refresh dialogs correctly, fixes #1866 Feb 9, 2021

fpabl0 added a commit to fpabl0/fyne that referenced this pull request Feb 9, 2021

fix file dialog resize method, ensure popup overlays are refreshed wh…

a7abe93

…en windows is created and resized, fixes fyne-io#1692 when fyne-io#1931 is landed

fpabl0 mentioned this pull request Feb 9, 2021

Ensure Popups are resized in newly created window and when window is resized, fixes #1692 #1932

Merged

7 tasks

andydotxyz requested changes Feb 10, 2021

View reviewed changes

dialog/base.go Outdated Show resolved Hide resolved

dialog/base.go Outdated Show resolved Hide resolved

widget/popup.go Outdated Show resolved Hide resolved

widget/popup.go Show resolved Hide resolved

call dialog.Refresh before dialog's popup is shown to avoid possible …

fc8854e

…flicker, remove useless comments

toaster requested changes Feb 15, 2021

View reviewed changes

fpabl0 added 2 commits February 16, 2021 00:38

Merge branch 'develop' into fix/1866

822dee2

added themedBackground widget to use it as dialog background, added a…

7414f5b

…nd updated dialog's tests

fpabl0 commented Feb 16, 2021

View reviewed changes

dialog/base.go Show resolved Hide resolved

fix popUpRenderer Layout (to respect content min size) and Refresh (t…

8ff886f

…o respect theme and canvas size changes) methods, fix popup tests

toaster requested changes Feb 17, 2021

View reviewed changes

dialog/base.go Outdated Show resolved Hide resolved

update themedBackgroundRenderer functions to respect fyne style guide…

f9a9ff6

…lines

toaster previously approved these changes Feb 19, 2021

View reviewed changes

move Refresh called in dialog.Show to popup.Show, fix layout issue wh…

9e6b35a

…en theme change in widget.PopUp, added popup tests

fpabl0 dismissed toaster’s stale review via 9e6b35a February 19, 2021 08:50

toaster previously approved these changes Feb 20, 2021

View reviewed changes

fix broken progress dialogs due to the last change

f34f979

fpabl0 dismissed toaster’s stale review via f34f979 February 21, 2021 21:35

andydotxyz approved these changes Feb 22, 2021

View reviewed changes

toaster approved these changes Feb 23, 2021

View reviewed changes

andydotxyz merged commit 7668395 into fyne-io:develop Feb 23, 2021

fpabl0 deleted the fix/1866 branch February 26, 2021 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh dialogs correctly, fixes #1866 #1931

Refresh dialogs correctly, fixes #1866 #1931

fpabl0 commented Feb 9, 2021 •

edited

fpabl0 commented Feb 9, 2021 •

edited

toaster left a comment

toaster commented Feb 15, 2021

fpabl0 commented Feb 15, 2021 •

edited

toaster commented Feb 15, 2021

andydotxyz commented Feb 15, 2021

andydotxyz commented Feb 15, 2021

fpabl0 Feb 16, 2021

andydotxyz Feb 18, 2021

fpabl0 Feb 18, 2021

fpabl0 Feb 18, 2021

andydotxyz Feb 19, 2021

fpabl0 Feb 19, 2021

andydotxyz Feb 20, 2021

andydotxyz Feb 20, 2021

andydotxyz Feb 20, 2021

fpabl0 Feb 20, 2021

toaster commented Feb 17, 2021

andydotxyz commented Feb 18, 2021

fpabl0 commented Feb 18, 2021

fpabl0 commented Feb 19, 2021 •

edited

toaster left a comment

andydotxyz commented Feb 21, 2021

fpabl0 commented Feb 21, 2021

fpabl0 commented Feb 21, 2021

andydotxyz commented Feb 22, 2021

andydotxyz commented Feb 22, 2021

toaster commented Feb 23, 2021

Refresh dialogs correctly, fixes #1866 #1931

Refresh dialogs correctly, fixes #1866 #1931

Conversation

fpabl0 commented Feb 9, 2021 • edited

Description:

Checklist:

fpabl0 commented Feb 9, 2021 • edited

toaster left a comment

Choose a reason for hiding this comment

toaster commented Feb 15, 2021

fpabl0 commented Feb 15, 2021 • edited

toaster commented Feb 15, 2021

andydotxyz commented Feb 15, 2021

andydotxyz commented Feb 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toaster commented Feb 17, 2021

andydotxyz commented Feb 18, 2021

fpabl0 commented Feb 18, 2021

fpabl0 commented Feb 19, 2021 • edited

toaster left a comment

Choose a reason for hiding this comment

andydotxyz commented Feb 21, 2021

fpabl0 commented Feb 21, 2021

fpabl0 commented Feb 21, 2021

andydotxyz commented Feb 22, 2021

andydotxyz commented Feb 22, 2021

toaster commented Feb 23, 2021

fpabl0 commented Feb 9, 2021 •

edited

fpabl0 commented Feb 9, 2021 •

edited

fpabl0 commented Feb 15, 2021 •

edited

fpabl0 commented Feb 19, 2021 •

edited