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.

Sign up for GitHub

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

Jump to bottom

Refresh dialogs correctly, fixes #1866 #1931

Merged
merged 8 commits into from Feb 23, 2021
Merged

Refresh dialogs correctly, fixes #1866 #1931

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0afcea3
fix modalPopUpRenderer refresh and refresh dialog when it is shown to…
fpabl0 Feb 9, 2021
fc8854e
call dialog.Refresh before dialog's popup is shown to avoid possible …
fpabl0 Feb 10, 2021
822dee2
Merge branch 'develop' into fix/1866
fpabl0 Feb 16, 2021
7414f5b
added themedBackground widget to use it as dialog background, added a…
fpabl0 Feb 16, 2021
8ff886f
fix popUpRenderer Layout (to respect content min size) and Refresh (t…
fpabl0 Feb 16, 2021
f9a9ff6
update themedBackgroundRenderer functions to respect fyne style guide…
fpabl0 Feb 18, 2021
9e6b35a
move Refresh called in dialog.Show to popup.Show, fix layout issue wh…
fpabl0 Feb 19, 2021
f34f979
fix broken progress dialogs due to the last change
fpabl0 Feb 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 43 additions & 11 deletions dialog/base.go
View file
Open in desktop
Expand Up @@ -40,7 +40,7 @@ type dialog struct {
desiredSize fyne.Size

win *widget.PopUp
bg *canvas.Rectangle
bg *themedBackground
fpabl0 marked this conversation as resolved.
Show resolved Hide resolved
content, label fyne.CanvasObject
dismiss *widget.Button
parent fyne.Window
Expand Down Expand Up @@ -121,7 +121,6 @@ func (d *dialog) Show() {
}

func (d *dialog) Refresh() {
d.applyTheme()
d.win.Refresh()
}

Expand Down Expand Up @@ -165,12 +164,6 @@ func (d *dialog) SetOnClosed(closed func()) {
}
}

func (d *dialog) applyTheme() {
r, g, b, _ := theme.BackgroundColor().RGBA()
bg := &color.NRGBA{R: uint8(r), G: uint8(g), B: uint8(b), A: 230}
d.bg.FillColor = bg
}

func (d *dialog) hideWithResponse(resp bool) {
d.win.Hide()
if d.callback != nil {
Expand All @@ -179,7 +172,7 @@ func (d *dialog) hideWithResponse(resp bool) {
}

func (d *dialog) setButtons(buttons fyne.CanvasObject) {
d.bg = canvas.NewRectangle(theme.BackgroundColor())
d.bg = newThemedBackground()
d.label = widget.NewLabelWithStyle(d.title, fyne.TextAlignLeading, fyne.TextStyle{Bold: true})

var content fyne.CanvasObject
Expand Down Expand Up @@ -228,6 +221,45 @@ func newButtonList(buttons ...*widget.Button) fyne.CanvasObject {
return list
}

// ===============================================================
// ThemedBackground
// ===============================================================

type themedBackground struct {
widget.BaseWidget
}

func newThemedBackground() *themedBackground {
t := &themedBackground{}
t.ExtendBaseWidget(t)
return t
}

func (t *themedBackground) CreateRenderer() fyne.WidgetRenderer {
t.ExtendBaseWidget(t)
rect := canvas.NewRectangle(theme.BackgroundColor())
return &themedBackgroundRenderer{rect, []fyne.CanvasObject{rect}}
}

type themedBackgroundRenderer struct {
rect *canvas.Rectangle
objects []fyne.CanvasObject
}

func (renderer *themedBackgroundRenderer) Objects() []fyne.CanvasObject { return renderer.objects }
func (renderer *themedBackgroundRenderer) Destroy() {}
func (renderer *themedBackgroundRenderer) Layout(size fyne.Size) { renderer.rect.Resize(size) }
func (renderer *themedBackgroundRenderer) MinSize() fyne.Size { return renderer.rect.MinSize() }
func (renderer *themedBackgroundRenderer) Refresh() {
fpabl0 marked this conversation as resolved.
Show resolved Hide resolved
r, g, b, _ := theme.BackgroundColor().RGBA()
bg := &color.NRGBA{R: uint8(r), G: uint8(g), B: uint8(b), A: 230}
renderer.rect.FillColor = bg
}

// ===============================================================
// DialogLayout
// ===============================================================

type dialogLayout struct {
d *dialog
}
Expand All @@ -236,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()
Copy link
Member Author

Choose a reason for hiding this comment

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

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:

Captura de Pantalla 2021-02-16 a la(s) 01 36 49

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I think you are describing widget.NewModalPopUp

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


// icon
iconHeight := padHeight*2 + l.d.label.MinSize().Height*2 - theme.Padding()
Expand All @@ -256,7 +288,7 @@ func (l *dialogLayout) Layout(obj []fyne.CanvasObject, size fyne.Size) {

func (l *dialogLayout) MinSize(obj []fyne.CanvasObject) fyne.Size {
contentMin := obj[2].MinSize()
btnMin := obj[3].MinSize().Max(obj[3].Size())
btnMin := obj[3].MinSize()

width := fyne.Max(fyne.Max(contentMin.Width, btnMin.Width), obj[4].MinSize().Width) + padWidth
height := contentMin.Height + btnMin.Height + l.d.label.MinSize().Height + theme.Padding() + padHeight*2
Expand Down
48 changes: 48 additions & 0 deletions dialog/base_test.go
View file
Open in desktop
Expand Up @@ -48,3 +48,51 @@ func TestShowCustom_Resize(t *testing.T) {
d.Show()
assert.Equal(t, size, d.(*dialog).win.Content.Size().Add(fyne.NewSize(theme.Padding()*2, theme.Padding()*2)))
}

func TestCustom_ApplyThemeOnShow(t *testing.T) {
test.NewApp()
defer test.NewApp()
w := test.NewWindow(canvas.NewRectangle(color.Transparent))
w.Resize(fyne.NewSize(200, 300))

label := widget.NewLabel("Content")
label.Alignment = fyne.TextAlignCenter
d := NewCustom("Title", "OK", label, w)

test.ApplyTheme(t, test.Theme())
d.Show()
test.AssertImageMatches(t, "dialog-onshow-theme-default.png", w.Canvas().Capture())
d.Hide()

test.ApplyTheme(t, test.NewTheme())
d.Show()
test.AssertImageMatches(t, "dialog-onshow-theme-changed.png", w.Canvas().Capture())
d.Hide()

test.ApplyTheme(t, test.Theme())
d.Show()
test.AssertImageMatches(t, "dialog-onshow-theme-default.png", w.Canvas().Capture())
d.Hide()
}

func TestCustom_ResizeOnShow(t *testing.T) {
test.NewApp()
defer test.NewApp()
w := test.NewWindow(canvas.NewRectangle(color.Transparent))
size := fyne.NewSize(200, 300)
w.Resize(size)

label := widget.NewLabel("Content")
label.Alignment = fyne.TextAlignCenter
d := NewCustom("Title", "OK", label, w).(*dialog)

d.Show()
assert.Equal(t, size, d.win.Size())
d.Hide()

size = fyne.NewSize(500, 500)
w.Resize(size)
d.Show()
assert.Equal(t, size, d.win.Size())
d.Hide()
}
4 changes: 2 additions & 2 deletions dialog/file_test.go
View file
Open in desktop
Expand Up @@ -387,7 +387,7 @@ func TestFileFilters(t *testing.T) {
count++
}
}
assert.Equal(t, 3, count)
assert.Equal(t, 5, count)

f.SetFilter(storage.NewMimeTypeFileFilter([]string{"image/jpeg"}))

Expand All @@ -412,7 +412,7 @@ func TestFileFilters(t *testing.T) {
count++
}
}
assert.Equal(t, 4, count)
assert.Equal(t, 6, count)
}

func TestFileFavorites(t *testing.T) {
Expand Down
Binary file modified dialog/testdata/color/dialog_expanded_theme_default.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added dialog/testdata/dialog-onshow-theme-changed.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added dialog/testdata/dialog-onshow-theme-default.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.