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

dialog.NewProgressInfinite is deprecated, but dialog.NewCustom isn't equivalent #2782

Closed
e1fueg0 opened this issue Feb 14, 2022 · 21 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@e1fueg0
Copy link

e1fueg0 commented Feb 14, 2022

Is your feature request related to a problem? Please describe:

dialog.NewProgressInfinite() uses a trick to get rid of dismiss button, but dialog.NewCustom() doens't give such possibility.

Is it possible to construct a solution with the existing API?

Still use dialog.NewProgressInfinite().

Describe the solution you'd like to see:

The simplest way is to not create dismiss button if dismiss string parameter of dialog.NewCustom() is empty.
Or maybe version like dialog.NewCustomWithButtons() or event expose setButtons() method.

@andydotxyz
Copy link
Member

Thanks for this. I agree with the general point, though not sure about setButtons() as that isn't a great API even for internal usage.
Perhaps dialog.NewCustomWithoutButtons(), or as you say dialog.NewCustomWithButtons(nil)...

@andydotxyz andydotxyz added the enhancement New feature or request label Feb 16, 2022
@andydotxyz andydotxyz added the blocker Items that would block a forthcoming release label Feb 16, 2022
@andydotxyz
Copy link
Member

Marking this as blocker so that it is in place for at least one release before deprecations may be removed.

@e1fueg0
Copy link
Author

e1fueg0 commented Feb 19, 2022

By the way, @andydotxyz , why not leave dialog.NewProgressInfinite(), but refactor it using the new API you were talking about?
Progress dialog creation using a single call is very convenient to use.

@Jacalz
Copy link
Member

Jacalz commented Feb 19, 2022

We deprecated dialog.NewProgressInfinite() because we decided (or perhaps "came to the conclusion" would be a better choice of words) that it is a poor design choice to pop up a progress bar in a dialog, and thus making it impossible to use the rest of the user interface at the same time. It is not something I think we should promote that developers use in their applications. If there is a good use-case to have a dialog without buttons, then I'm entirely in favour of that solution instead.

@e1fueg0
Copy link
Author

e1fueg0 commented Feb 19, 2022

Hm. There are a lot of tasks which take several seconds to be finished, but it's too complex or too long to stop them, or authors see no need to stop them.

because we decided that it is a poor design choice

In fact, you know, some developers may not agree with this and they will still create infinite progress dialogs, so may be you can provide them with a standard implementation, specially when you already have the API.

@Jacalz
Copy link
Member

Jacalz commented Feb 19, 2022

First of all, I do want this to turn into a long argument back and forth. Let me just clarify my last comment based on your answers. My last comment should have provided some more clarification to begin with.

Hm. There are a lot of tasks which take several seconds to be finished, but it's too complex or too long to stop them, or authors see no need to stop them.

It's not about long tasks that can't be stopped, or such. If someone has a dialog that pops up with a progress dialog, there is often a better way to handle the problem. Go has excellent support for threading and doing things concurrently. If a developer decides that they really need to do this, they can simply make a custom dialog with the same functionality (or so we though).

because we decided that it is a poor design choice

In fact, you know, some developers may not agree with this and they will still create infinite progress dialogs, so may be you can provide them with a standard implementation, specially when you already have the API.

That's not really how you create a good API. We need to provide an API with sensible defaults, by iterating and improving as we go. If that means removing something that we think no longer fits in, we need to do so. There will always be people that use APIs in a way that wasn't anticipated or intended, but that doesn't mean that we should make that easier.

I hope that this clarifies why we decided to deprecate it. Is there any other use-case for dialogs without buttons, except for progress dialogs?

@e1fueg0
Copy link
Author

e1fueg0 commented Feb 19, 2022

Ok, ok, it's your framework, it's your decisions, no problem =)

@andydotxyz
Copy link
Member

Ok, ok, it's your framework, it's your decisions, no problem =)

Apologies @e1fueg0 I think this may have been responded to a bit abruptly. Your comment is a good one and we need to think about it.

@Jacalz The comment on the deprecation notice states that instead of infinite progress the custom dialog should be used, but as noted above it's not the same, as the buttons cannot be removed.
I agree with the intent to remove this discouraged API - however we imply a direct replacement that does not seem to be the case. Perhaps our documentation is misleading...

@Jacalz
Copy link
Member

Jacalz commented Feb 22, 2022

Yes. We (or perhaps mostly I) were certainly wrong in that assumption. It's absolutely something we should fix.

Sorry if that didn't come across in my earlier messages. I just tried to explain why we came to the decision that we did, and I might have worded those answers in a poor way.

@e1fueg0
Copy link
Author

e1fueg0 commented Feb 22, 2022

No problems at all, guys, you're doing a really great job.

@andydotxyz
Copy link
Member

Thanks @e1fueg0.
The question then for @Jacalz or the team is should the documentation to be fixed or should we continue to support some sort of way to have a "no button dialog" that simply does not have a default form such as has been deprecated so we continue to discourage it's usage?

@Jacalz
Copy link
Member

Jacalz commented Feb 23, 2022

If there are more use cases for the "no button dialog", then I think it could be a viable solution. If not, I'm not quite sure if it is worth adding just for creating progress dialogs though. Then I'd say we update the documentation.

@andydotxyz
Copy link
Member

I can see that it is easier to imagine to think it will be useful than to say it is always a bad thing ;)

gitcoindev added a commit to gitcoindev/FairPass that referenced this issue Mar 15, 2022
Linter complains about dialog.NewProgressInfinite() deprecation:
dialog.NewProgressInfinite is deprecated: Create a new custom dialog with a widget.ProgressBarInfinite() inside. (SA1019)

there is still an open ticket fyne-io/fyne#2782 on difference against the new implementation.

Exclude these from linter and consider refactoring.
gitcoindev added a commit to gitcoindev/FairPass that referenced this issue Mar 18, 2022
Linter complains about dialog.NewProgressInfinite() deprecation:
dialog.NewProgressInfinite is deprecated: Create a new custom dialog with a widget.ProgressBarInfinite() inside. (SA1019)

there is still an open ticket fyne-io/fyne#2782 on difference against the new implementation.

Exclude these from linter and consider refactoring.
asabya pushed a commit to fairDataSociety/FairPass that referenced this issue Apr 22, 2022
* workflows: Implement linux build.

Implement a build workflow for Linux.

Ref: #1

* Update build.yml

* Create test.yml

* build.yml: Add windows build

Ref: #2

* Update test.yml

* Update build.yml

* build.yml: Add icons

* Update build.yml

* Added android build to build workflow

* Create static-code-analysis.yml

* Remove darwin build

Requires Xcode license

* Update build.yml

* Add dependencies to static code analysis workflow

* Update static-code-analysis.yml

* Update readme with CI statuses

* internal: Exclude dialog.NewProgressInfinite() from linter deprecation.

Linter complains about dialog.NewProgressInfinite() deprecation:
dialog.NewProgressInfinite is deprecated: Create a new custom dialog with a widget.ProgressBarInfinite() inside. (SA1019)

there is still an open ticket fyne-io/fyne#2782 on difference against the new implementation.

Exclude these from linter and consider refactoring.

* git.mod/sum: Update dependencies.

* loginView: display progress bar when connecting to the Bee client.

* passwordsView: Increase domains and user table column width.

The current table width for domains and users turned out to be too
narrow in most cases. Increase the width of the two columns for better
readability.

* passwordCreate: Implement batch passwords import from Google Chrome/
Brave export.

It is possible to export all Google Chrome/Brave passwords to a CSV
file.

The expected input format of the CSV file is:

name,url,username,password
discord.com,https://discord.com/login,youremail@gmail.com,your_secure_password
github.com,https://github.com/session,youremail@gmail.com,your_secure_password

The first line will be omitted.

Ref: #8

* build.yml: Refactor build job to use GitHub matrix.

Reference: https://docs.github.com/en/actions/using-jobs/using-a-build-matrix-for-your-jobs

* release.yml: Add release job with GitHub matrix support.

Reference: https://docs.github.com/en/actions/using-jobs/using-a-build-matrix-for-your-jobs
@andydotxyz
Copy link
Member

This is frustrating but we decided not a blocker.
We will add some sort of no button custom dialog in a future release, but first we want to get the delayed 2.2.0 done.

@andydotxyz andydotxyz removed the blocker Items that would block a forthcoming release label May 6, 2022
@Jacalz
Copy link
Member

Jacalz commented May 6, 2022

Indeed. This is technically not a blocker until it is being removed. We’ll get this sorted way before then.

@Jacalz
Copy link
Member

Jacalz commented Jan 8, 2023

We discussed improving custom dialogs on our call this Friday and it seems like we have a path forward. I will try to get that sorted, and thus also solve this, when I have some time over.

@yms2772
Copy link

yms2772 commented Feb 23, 2023

Any update about this discussion?
Custom dialog without buttons are definitely useful, so the fyne framework should give us options to handle dialog freely.
As an example, *os.File or *http.Response is giving the user the freedom to Close() even if the memory leaks are expected.

I think it would be useful in a goroutine like the code below.

go func() {
    noBtn := dialogs.NewCustomWithoutButton(SOME CONTENTS)
    defer noBtn.Close() // or defer noBtn.Hide()

    // any progress
}

@Jacalz
Copy link
Member

Jacalz commented Feb 23, 2023

The update is my comment above. Have not had any time to start working on this yet...

@yms2772
Copy link

yms2772 commented Feb 24, 2023

I implemented a dialog without buttons by making some modifications to the base.go file.
I'll use this until it's officially released.
master...yms2772:fyne:master

test

@Jacalz
Copy link
Member

Jacalz commented Mar 5, 2023

I have opened #3703. Feel free to test and see if it suits your use cases.

@Jacalz
Copy link
Member

Jacalz commented Mar 31, 2023

I will close this as completed now that the PR has been merged into the develop branch.

@Jacalz Jacalz closed this as completed Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants