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

Support custom code on file transfer by dropping onto the window #123

Merged
merged 3 commits into from
Oct 22, 2023

Conversation

GalihFajar
Copy link

Description:

When file is being dropped into the window, instead of using default generated code, a prompt is shown whether user wants to use his own or use the default (generated) code. The prompt is very similar when custom code option is ticked when trying to send the file using default Add content to send button.

Example:
image

Fixes: #108

Checklist:

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

@Jacalz Jacalz self-requested a review October 18, 2023 12:54
Copy link
Owner

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I have one idea for how we might want to do it instead.

@@ -31,6 +31,8 @@ func Create(app fyne.App, window fyne.Window) *container.AppTabs {
tabs.SelectIndex(0)
}

send.client.CustomCode = true
Copy link
Owner

Choose a reason for hiding this comment

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

How about instead bringing up a dialog asking the user just if they want to use a custom code or not?
Maybe even a setting to control if the user want the question to pop up or not? If the user says yes, we can set CustomCode = true and the backend wouldn't have to know then if it is a drop or not.

Copy link
Author

Choose a reason for hiding this comment

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

Sure thing, would be consistent & intuitive too. Though I might need more time to explore Fyne though 😅 .

Copy link
Owner

Choose a reason for hiding this comment

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

Cool. I think most of what needs doing should be similar to code that is already there. However, you are of course more than welcome to ask if you need any help :)

Copy link
Author

Choose a reason for hiding this comment

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

Hi @Jacalz, I've made an update. Now when file is being dropped, a prompt is shown. But I can't set the CustomCode properly back to false after it's being set to true:

confirm := dialog.NewConfirm("Create using custom code", "Create using custom code?", func(custom bool) {
	send.client.CustomCode = custom
	send.newTransfer(uris)
        send.client.CustomCode = false // -> won't work because the newTransfer is being done concurrently (I guess)
         }, window)
confirm.Show()

This will cause a side effect: the Use a custom code tick will not be ticked but the state of the send.client.CustomCode is still set to false.

So in order to mitigate this problem, I'm updating the state of the whole window, by newly creating it every time the Add content to send button is tapped:

contentToSend := &widget.Button{Text: "Add content to send", Icon: theme.ContentAddIcon(), OnTapped: func() {
		codeChoice := &widget.Check{Text: "Use a custom code", OnChanged: s.onCustomCode, Checked: s.client.CustomCode}
		choiceContent := container.NewGridWithColumns(1, fileChoice, directoryChoice, textChoice, codeChoice)
		s.contentPicker = dialog.NewCustom("Pick a content type", "Cancel", choiceContent, window)
		s.contentPicker.Show()
	}}

I'm not sure about the performance or how proper the behavior is, so kindly give feedbacks on this.

Many thanks!

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks. The solution does look very good visually and from a usability standpoint. I will have a look at if the state problem you are talking about can be improved.

@GalihFajar GalihFajar marked this pull request as ready for review October 20, 2023 08:43
Copy link
Owner

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

I just have a few suggestions :)

Comment on lines 44 to 49
contentToSend := &widget.Button{Text: "Add content to send", Icon: theme.ContentAddIcon(), OnTapped: func() {
codeChoice := &widget.Check{Text: "Use a custom code", OnChanged: s.onCustomCode, Checked: s.client.CustomCode}
choiceContent := container.NewGridWithColumns(1, fileChoice, directoryChoice, textChoice, codeChoice)
s.contentPicker = dialog.NewCustom("Pick a content type", "Cancel", choiceContent, window)
s.contentPicker.Show()
}}
Copy link
Owner

Choose a reason for hiding this comment

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

You should be able to change back most of the code in this function and just update the state like this instead:

	contentToSend := &widget.Button{Text: "Add content to send", Icon: theme.ContentAddIcon(), OnTapped: func() {
		codeChoice.SetChecked(s.client.CustomCode)
		s.contentPicker.Show()
	}}

Copy link
Author

Choose a reason for hiding this comment

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

Updated, didn't know such simple function exists (and works well) 😅

Copy link
Owner

Choose a reason for hiding this comment

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

No problem. I'll review this tomorrow and hopefully merge :)

Comment on lines 35 to 40
confirm := dialog.NewConfirm("Create using custom code", "Create using custom code?", func(custom bool) {
send.client.CustomCode = custom
send.newTransfer(uris)
}, window)
confirm.Show()
})
Copy link
Owner

Choose a reason for hiding this comment

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

This can be simplified (and we can avoid some duplicated wording):

Suggested change
confirm := dialog.NewConfirm("Create using custom code", "Create using custom code?", func(custom bool) {
send.client.CustomCode = custom
send.newTransfer(uris)
}, window)
confirm.Show()
})
dialog.ShowConfirm("Custom Code", "Use a custom code?", func(custom bool) {
send.client.CustomCode = custom
send.newTransfer(uris)
}, window)
})

Copy link
Author

Choose a reason for hiding this comment

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

Updated

Copy link
Owner

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Sorry for the wait. Thanks for working on this. It looks great. I'll merge :)

@Jacalz Jacalz merged commit 1df86b1 into Jacalz:next Oct 22, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants