Added additional documentation and example code snippet. #2156

lazyhacker · 2021-04-11T04:55:08Z

Description:

Adding additional comment that the callback function is triggered for both Open and Cancel.

Fixes #(issue)

#2152

Checklist:

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

Where applicable:

Jacalz

I very much appreciate the PR. There are a few changes that I would suggest, but thanks for taking on this effort 🙂

This kind of documentation should be added to the other file dialogs as fell (Save and Open) as they will be called with nil values as well.

Jacalz · 2021-04-11T07:43:13Z

dialog/folder.go

+//    a := app.New()
+//    w := a.NewWindow("example")
+//    button := widget.NewButtonWithIcon("", theme.FolderIcon(),
+//       func() {
+//           dialog.ShowFolderOpen(
+//               func (uri fyne.ListableURI, err error) {
+//                   // Cancel will pass a nil value.
+//                   if uri == nil || err != nil {
+//                       return
+//                   }
+//                   fmt.Println("URI", uri)
+//               }, w)
+//       }
+//    )
+//


I really appreciate the effort, but you are not supposed to add code examples to the API docs in this way. See this link for an example https://blog.golang.org/examples on how it should be done for interactive examples.

However, we do have the fyne_demo application so I don't know if we really should add this kind of example in (someone else will have to comment on that) because adding it to one function would mean that we open up the possibility of adding to much more functions in the future.

The interactive examples are normally able to be executed as a test and in the browser and I'm not sure how to get something like that to work in this case where it's rendering a GUI dialog box. That's why I just put in in a code snippet as an example similar to what Go's doc do for similar cases (e.g. https://golang.org/pkg/net/).

It's perfectly alright with me if this is not the right fit for the project so please feel free to reject the PR.

If the fyne_demo has the illustrative snippet of code, my suggestion would be to add a direct reference to it somehow in the doc to make it easy to find. A demo app's source code (especially one as complete as the demo) isn't as easily navigable IMHO so help to get to the right part would help.

Fyne code can be rendered in the playground in theory, but there is a 5sec timeout that I am sure means we can’t get the app compiled (you could check out the tools/playground package though).
With WASM aiming for 2.1 this may get much better :)

I am nervous about adding code examples like this (i.e. entire applications) because it includes so much outside the scope of this function. Linking to the fyne_demo project could be a nice way to do it, though those links may go stale as well.
I liked what was done with the clarification in the paragraph of text above the example, but feel that large examples like this would be better placed in the tutorials at https://developer.fyne.io.

dialog/folder.go

andydotxyz

The clarification of usage is a great addition, but let's not go down the path of what could be an application per-function

dialog/folder.go

andydotxyz · 2021-04-16T10:15:37Z

dialog/folder.go

+//    a := app.New()
+//    w := a.NewWindow("example")
+//    button := widget.NewButtonWithIcon("", theme.FolderIcon(),
+//       func() {
+//           dialog.ShowFolderOpen(
+//               func (uri fyne.ListableURI, err error) {
+//                   // Cancel will pass a nil value.
+//                   if uri == nil || err != nil {
+//                       return
+//                   }
+//                   fmt.Println("URI", uri)
+//               }, w)
+//       }
+//    )
+//


I am nervous about adding code examples like this (i.e. entire applications) because it includes so much outside the scope of this function. Linking to the fyne_demo project could be a nice way to do it, though those links may go stale as well.
I liked what was done with the clarification in the paragraph of text above the example, but feel that large examples like this would be better placed in the tutorials at https://developer.fyne.io.

Co-authored-by: Jacob <Jacalz@users.noreply.github.com>

Bring fork up-to-date

andydotxyz · 2021-04-24T11:59:52Z

dialog/folder.go

-// ShowFolderOpen creates and shows a file dialog allowing the user to choose a folder to open.
+// ShowFolderOpen creates and shows a file dialog allowing the user to choose a
+// folder to open. The callback function will run when the dialog closes.  The
+// URI will be nil when nothing is selected or cancelled.


The latest change made this slightly ambiguous sorry - "nothing is selected or cancelled".
I think "the user cancelled or nothing is selected" is clearer.

Jacalz

I agree with @andydotxyz on the wording. Also please update the other file/folder dialogs with the same note.

a nil value.

…nil value.

andydotxyz

Thank you so much for this clarification

andydotxyz · 2021-05-08T17:28:31Z

I'm so sorry that this dropped off my watch list.
It seems that GitHub stopped auto-running CI for new contributors and so I didn't get the usual notifications.

andydotxyz · 2021-05-08T17:32:04Z

I think this is waiting for your re-review @Jacalz if you could check it out :)

Jacalz

Sorry, I had forgotten about this. I had a minor thing that I found when re-reviewing this. Please remove the double spaces 🙂

Jacalz · 2021-05-08T19:18:35Z

dialog/file.go

 // The dialog will appear over the window specified when Show() is called.
 func NewFileSave(callback func(fyne.URIWriteCloser, error), parent fyne.Window) *FileDialog {
 	dialog := &FileDialog{callback: callback, parent: parent, save: true}
 	return dialog
 }

-// ShowFileOpen creates and shows a file dialog allowing the user to choose a file to open.
+// ShowFileOpen creates and shows a file dialog allowing the user to choose a
+// file to open. The callback function will run when the dialog closes.  The URI


Suggested change

// file to open. The callback function will run when the dialog closes. The URI

// file to open. The callback function will run when the dialog closes. The URI

It's grammatically incorrect with double spaces. There are a few more places where this occurs but I just made one comment about it.

There is actually a school of thinking that says double space is grammatically correct. However I agree that we have settled on single spacing.

I did not know that. It is very, very forbidden in Swedish at least 😅

…ce standard instead of double space standard after periods.

Jacalz

Looks good to me. Sorry for the long wait, but I never got a request for a re-review.

Added additional documentation and example code snippet.

6808a8c

Jacalz requested changes Apr 11, 2021

View reviewed changes

andydotxyz requested changes Apr 16, 2021

View reviewed changes

lazyhacker and others added 3 commits April 21, 2021 10:17

Update dialog/folder.go

12d2272

Co-authored-by: Jacob <Jacalz@users.noreply.github.com>

Merge pull request #1 from fyne-io/develop

93fa956

Bring fork up-to-date

Remove code snippet per review feedback.

1d4725e

andydotxyz requested changes Apr 24, 2021

View reviewed changes

Jacalz requested changes Apr 24, 2021

View reviewed changes

lazyhacker added 3 commits April 24, 2021 10:43

Clarified wording per review comments.

b142ab7

Add comment about when the callback function is called and when it gets

9551c9b

a nil value.

Added comment on when callback function is called and when it gets a …

9bf6bca

…nil value.

andydotxyz previously approved these changes Apr 26, 2021

View reviewed changes

Jacalz requested changes May 8, 2021

View reviewed changes

lazyhacker and others added 2 commits May 15, 2021 16:45

Merge branch 'fyne-io:develop' into develop

1b461a6

Changed spacing between sentences to single. Fyne uses the single spa…

a86eea6

…ce standard instead of double space standard after periods.

lazyhacker dismissed andydotxyz’s stale review via a86eea6 May 15, 2021 23:54

Jacalz approved these changes May 22, 2021

View reviewed changes

andydotxyz merged commit 8864cb4 into fyne-io:develop May 22, 2021

andydotxyz pushed a commit that referenced this pull request Jul 7, 2021

Added additional documentation and example code snippet. (#2156)

31dff03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added additional documentation and example code snippet. #2156

Added additional documentation and example code snippet. #2156

lazyhacker commented Apr 11, 2021

Jacalz left a comment

Jacalz Apr 11, 2021

lazyhacker Apr 11, 2021

andydotxyz Apr 11, 2021 •

edited

andydotxyz Apr 16, 2021

andydotxyz left a comment

andydotxyz Apr 16, 2021

andydotxyz Apr 24, 2021

Jacalz left a comment

andydotxyz left a comment

andydotxyz commented May 8, 2021

andydotxyz commented May 8, 2021

Jacalz left a comment

Jacalz May 8, 2021

andydotxyz May 8, 2021

Jacalz May 8, 2021

Jacalz left a comment

	// file to open. The callback function will run when the dialog closes. The URI
	// file to open. The callback function will run when the dialog closes. The URI

Added additional documentation and example code snippet. #2156

Added additional documentation and example code snippet. #2156

Conversation

lazyhacker commented Apr 11, 2021

Description:

Checklist:

Where applicable:

Jacalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz Apr 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacalz left a comment

Choose a reason for hiding this comment

andydotxyz left a comment

Choose a reason for hiding this comment

andydotxyz commented May 8, 2021

andydotxyz commented May 8, 2021

Jacalz left a 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

Jacalz left a comment

Choose a reason for hiding this comment

andydotxyz Apr 11, 2021 •

edited