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

Delay auto resolve skippable dialog #219

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Thaina
Copy link

@Thaina Thaina commented May 18, 2019

For #217

Copy link
Contributor

@stewartmiles stewartmiles 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 the patch. Mostly looks ok, added a few comments.

source/PlayServicesResolver/src/PlayServicesResolver.cs Outdated Show resolved Hide resolved
source/PlayServicesResolver/src/PlayServicesResolver.cs Outdated Show resolved Hide resolved
source/PlayServicesResolver/src/PlayServicesResolver.cs Outdated Show resolved Hide resolved
source/PlayServicesResolver/src/PlayServicesResolver.cs Outdated Show resolved Hide resolved
source/PlayServicesResolver/src/PlayServicesResolver.cs Outdated Show resolved Hide resolved
source/PlayServicesResolver/src/PlayServicesResolver.cs Outdated Show resolved Hide resolved
source/PlayServicesResolver/src/SettingsDialog.cs Outdated Show resolved Hide resolved
@stewartmiles
Copy link
Contributor

As I mentioned when I started looking at this, it would be nice to rebase and squash this change into a single commit to merge.

$ git rebase -i

Squash all commits after the first one in the list so they're all combined into one

Change the commit message to describe what this is doing (e.g https://chris.beams.io/posts/git-commit/ is a nice tutorial)

Replace your branch with the squashed commit

$ git push origin -f HEAD:master

@Thaina
Copy link
Author

Thaina commented May 22, 2019

@stewartmiles Doesn't the git could squash whenever you merge my branch into your branch?

https://github.blog/2016-04-01-squash-your-commits/

Or I am misunderstand this blog post?

Anyway I would fix everything as your comment first and would look about squashing or recommitting this when there was the last thing to be fixed

also use Resolver.AutomaticResolutionEnabled() to skip the popup

Update PlayServicesResolver.cs

Update PlayServicesResolver.cs

Delay auto resolve dialog

add AutoResolutionDelay
@Thaina
Copy link
Author

Thaina commented May 23, 2019

@stewartmiles I have tries to squash by tortoisegit, is this what you expected?

@stewartmiles
Copy link
Contributor

@Thaina typically I would use the command line to do this using git rebase -i origin/master that opens an editor and allows me to "squash" all commits in the series into a single commit i.e from origin/master..HEAD.

I think it's possible to do the same with tortoisegit https://stackoverflow.com/questions/12528854/how-to-perform-rebase-squash-using-tortoisegit

@Thaina
Copy link
Author

Thaina commented Jun 6, 2019

@stewartmiles I have already did that and the result is as I have committed here > 4f15dd7

@stewartmiles
Copy link
Contributor

I merged / rebased and squashed this commit into a local repo, the result is here stewartmiles@fe13245

At the moment this doesn't really work.

  • Since AlertModal uses EditorUtility.DisplayDialogComplex / EditorUtility.DisplayDialog it's not possible to update the dialog text for the count down.
  • If resolution is skipped, auto-resolution doesn't appear to happen again
  • After the timeout the alert dialog isn't closed

Any chance you built this and managed to test it?

@Thaina
Copy link
Author

Thaina commented Jun 30, 2019

I have fix it by using progressbar dialog. Now it could only cancel or wait to have it closed by itself

I could build the dll too. But it required to change something in gradle to let it be able to built in windows

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