* Changes direct usages of GUI.enabled to instead use EditorGUI.Begin… #207

MunchyYDL · 2017-08-21T12:06:47Z

…DisabledGroup() / EditorGUI.EndDisabledGroup()

Requirements

Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
All new code requires tests to ensure against regressions

Description of the Change

I have replaced all the possible direct usages of GUI.enabled with a corresponding set of calls to the EditorGUI.BeginDisabledGroup() and EditorGUI.EndDisabledGroup(). I have also used the code block style that I see you have used before, which places the code between the calls in a separate code block with { }, and indents the code making it easier to read.

I have also done two small other changes in this PR.

Renamed the file PUblishView.cs to PublishView.cs
Renamed the variable busy to isBusy in AuthenticationView.cs, to follow the naming in the other views.

Alternate Designs

Benefits

Possible Drawbacks

Applicable Issues

…DisabledGroup() / EditorGUI.EndDisabledGroup()

StanleyGoldman · 2017-08-21T12:10:47Z

Hey @MunchyYDL thanks for the pull request!

StanleyGoldman · 2017-08-21T12:12:47Z

Eek, some of your changes in PUblishView.cs 🤣 might conflict with some changes I have in #196.
But otherwise it's looking good initially.

MunchyYDL · 2017-08-21T13:14:50Z

Haha ok, I actually did some changes just to make it harder for you. ;)

StanleyGoldman

So what's really cool about BeginDisabledGroup/EndDisabledGroup is that you can use it to disable a whole lot of controls at the same time.

Check out what I did in this pending PR and see if you can copy what I did.

https://github.com/github-for-unity/Unity/blob/fixes/publish-view-disable-until-load/src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PUblishView.cs#L138-L237

StanleyGoldman · 2017-08-21T15:58:56Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/AuthenticationView.cs

-                if (busy) GUI.enabled = false;
-                username = EditorGUILayout.TextField(usernameLabel ,username, Styles.TextFieldStyle);
-                GUI.enabled = true;
+                EditorGUI.BeginDisabledGroup(isBusy);


We can use a wider scope here.

I will look at this as well, didn't want to change scope before, as I didn't know what you guys wanted.

StanleyGoldman · 2017-08-21T15:59:11Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/AuthenticationView.cs

-                if (busy) GUI.enabled = false;
-                two2fa = EditorGUILayout.TextField(twofaLabel, two2fa, Styles.TextFieldStyle);
-                GUI.enabled = true;
+                EditorGUI.BeginDisabledGroup(isBusy);


We can use a wider scope here.

StanleyGoldman · 2017-08-21T16:04:26Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/PublishView.cs

-                    GUI.enabled = !isBusy;
-                    selectedOwner = EditorGUILayout.Popup(0, owners);
-                    GUI.enabled = true;
+                    EditorGUI.BeginDisabledGroup(isBusy);


We can use a wider scope here.

StanleyGoldman

Sorry to be crazy about the formatting. It just makes it easier to validate the good work you did.

StanleyGoldman · 2017-08-21T16:10:59Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/HistoryView.cs

@@ -207,7 +207,8 @@ private void RefreshLog()
            {
                if (Repository != null)
                {
-                    GitClient.Log().ThenInUI((success, log) => {
+                    GitClient.Log().ThenInUI((success, log) =>
+                    {


I think you accidentally re-formatted this chunk of code here, could you restore it?

Sure, will fix that. I use tabs for spacing normally, so I had to reformat the code after I had done my changes, and thought that you guys just had missed this piece of formatting before. I will revert it. :)

StanleyGoldman · 2017-08-21T16:11:24Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/HistoryView.cs

@@ -569,7 +585,8 @@ private void RevertCommit()
            {
                Repository
                    .Revert(selection.CommitID)
-                    .FinallyInUI((success, e) => {
+                    .FinallyInUI((success, e) =>
+                    {


StanleyGoldman · 2017-08-21T16:11:28Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/HistoryView.cs

@@ -699,7 +716,8 @@ private void Pull()
                            // (either git rebase --abort or git merge --abort)
                        }
                    }, true)
-                    .FinallyInUI((success, e) => {
+                    .FinallyInUI((success, e) =>
+                    {


StanleyGoldman · 2017-08-21T16:11:39Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/HistoryView.cs

@@ -722,7 +740,8 @@ private void Push()
            var remote = Repository.CurrentRemote.HasValue ? Repository.CurrentRemote.Value.Name : String.Empty;
            Repository
                .Push()
-                .FinallyInUI((success, e) => {
+                .FinallyInUI((success, e) =>
+                {


StanleyGoldman · 2017-08-21T16:12:54Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/HistoryView.cs

@@ -744,7 +763,8 @@ private void Fetch()
            var remote = Repository.CurrentRemote.HasValue ? Repository.CurrentRemote.Value.Name : String.Empty;
            Repository
                .Fetch()
-                .FinallyInUI((success, e) => {
+                .FinallyInUI((success, e) =>
+                {


Same here

... Please don't hate me 🙈

Haha, no worries, I understand that you want this fixed.

MunchyYDL · 2017-08-23T13:20:36Z

@StanleyGoldman: I have made some changes, can you please have a look and see if this is more in line with what you wanted? :)

StanleyGoldman · 2017-08-28T11:19:47Z

Hey @MunchyYDL sorry to leave you hanging. I will take a look at your stuff today.

MunchyYDL · 2017-08-28T11:22:15Z

No worries @StanleyGoldman, I had some other things to work on as well. :)

…ed-to-instead-use-editorgui-begin

StanleyGoldman

Looking good, a few more nit picks as we are ready to go.

StanleyGoldman · 2017-08-28T13:01:32Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/AuthenticationView.cs

+                GUILayout.BeginHorizontal();
+                {
+                    GUILayout.FlexibleSpace();
+                    if (GUILayout.Button(loginButton) || (GUI.enabled && enterPressed))


Hmm, I think we have to do something about this GUI.enabled here.

StanleyGoldman · 2017-08-28T13:02:07Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/AuthenticationView.cs

+                            Redraw();
+                        }
+
+                        if (GUILayout.Button(twofaButton) || (GUI.enabled && enterPressed))


Same scenario here.

StanleyGoldman · 2017-08-28T13:03:45Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/HistoryView.cs

                }
+                EditorGUI.EndDisabledGroup();
                GUI.enabled = enabled;


This should go away.

StanleyGoldman · 2017-08-28T13:04:00Z

src/UnityExtension/Assets/Editor/GitHub.Unity/UI/HistoryView.cs

@@ -311,16 +311,19 @@ private void DoOfferToInitializeRepositoryGUI()
                GUILayout.FlexibleSpace();

                var enabled = GUI.enabled;


This variable should go away too,

…into fixes/gui_enabled

MunchyYDL · 2017-08-28T21:06:27Z

So, now I had some time to look at this again!

Wasn't aware of you pushing to my repo, I hope that doesn't mess up anything about the history in my branch, as I didn't notice it before I had done my last commit locally. :)

AuthenticationView.cs

Didn't think this through I guess, but now I have replaced GUI.enabled with a check for !isBusy, to prevent multiple invocations of the button handlers in both the "Normal" and "2FA" login code.

HistoryView.cs

What can I say? Well, it's gone now. :)

MunchyYDL · 2017-08-28T21:10:02Z

Oh, and one more thing, what's the difference of the two appveyor checks?

As you can see, I'm a bit new to this whole PR thing, so I hope I'm not giving you too much trouble in "helping" you guys with this. :)

shana · 2017-08-29T17:26:58Z

@MunchyYDL One of the builds is for your branch, and the other is for your branch merged with the target branch (master in this case). Since your branch is in a forked repo, AppVeyor isn't running a build for it, and it only does the combined branch+master build. I just can't convince the UI to ignore this check if the branch is on a fork so it makes for a bit of confusing status checks, but it isn't a problem.

shana · 2017-08-29T17:52:44Z

Wasn't aware of you pushing to my repo, I hope that doesn't mess up anything about the history in my branch, as I didn't notice it before I had done my last commit locally. :)

Ah, yeah, sorry, that's just a thing we're used to doing, updating branches with the latest master so it's all up to date, it's kind of routine for all branches (they can't be merged if they're not up to date), but we shouldn't do it for branches we don't own without letting you know first (and preferably you do it, not us). I hope that didn't cause any issues on your end!

shana

This looks great! 🎉

StanleyGoldman · 2017-08-30T12:24:14Z

We are happy to help... but also, you started helping first! 🍻

There are a lot of reasons why.. Mostly, we can't fix everything.
And this time you are doing something simple, next time it will be a bit harder.

Hopefully soon you'll be taking whole features off our hands.. 😈

StanleyGoldman · 2017-08-30T12:53:49Z

FYI, there is a checkbox when you were creating the pull request that allows us to push just to that branch in your fork.

https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

MunchyYDL · 2017-08-30T15:48:49Z

Thanks for your help and nice comments!

I've been using both Git and Unity before, so I thought this project was a nice combination to help me further my understanding of both, and be able to submit my first PR to an open project.

* Changes direct usages of GUI.enabled to instead use EditorGUI.Begin…

f0f7d55

…DisabledGroup() / EditorGUI.EndDisabledGroup()

StanleyGoldman added the ready-for-review label Aug 21, 2017

StanleyGoldman suggested changes Aug 21, 2017

View reviewed changes

StanleyGoldman added work-in-progress and removed ready-for-review labels Aug 21, 2017

MunchyYDL added 3 commits August 23, 2017 11:56

* Merge from master

8b2312c

- Reverted the formatting for lambdas.

ad40bb9

* Moved the Begin/EndDisabledGroup to a wider scope.

f46c1d6

Merge branch 'master' into pr/207--changes-direct-usages-of-gui-enabl…

a88c32c

…ed-to-instead-use-editorgui-begin

StanleyGoldman approved these changes Aug 28, 2017

View reviewed changes

StanleyGoldman suggested changes Aug 28, 2017

View reviewed changes

StanleyGoldman mentioned this pull request Aug 28, 2017

Implement Git LFS workaround affecting Pull Request clones github/VisualStudio#1205

Open

MunchyYDL added 2 commits August 28, 2017 22:42

- Removed the last occurrences of GUI.enabled

bbd969f

Merge branch 'fixes/gui_enabled' of https://github.com/MunchyYDL/Unity …

e66dfc4

…into fixes/gui_enabled

shana approved these changes Aug 29, 2017

View reviewed changes

StanleyGoldman approved these changes Aug 30, 2017

View reviewed changes

Merge branch 'master' into fixes/gui_enabled

57da18f

StanleyGoldman merged commit 784d624 into github-for-unity:master Aug 30, 2017

MunchyYDL deleted the fixes/gui_enabled branch August 30, 2017 15:48

		@@ -311,16 +311,19 @@ private void DoOfferToInitializeRepositoryGUI()
		GUILayout.FlexibleSpace();

		var enabled = GUI.enabled;

* Changes direct usages of GUI.enabled to instead use EditorGUI.Begin… #207

* Changes direct usages of GUI.enabled to instead use EditorGUI.Begin… #207

Uh oh!

Conversation

MunchyYDL commented Aug 21, 2017

Requirements

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Applicable Issues

Uh oh!

StanleyGoldman commented Aug 21, 2017

Uh oh!

StanleyGoldman commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MunchyYDL commented Aug 21, 2017

Uh oh!

StanleyGoldman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StanleyGoldman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MunchyYDL commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StanleyGoldman commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MunchyYDL commented Aug 28, 2017

Uh oh!

StanleyGoldman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MunchyYDL commented Aug 28, 2017

AuthenticationView.cs

HistoryView.cs

Uh oh!

MunchyYDL commented Aug 28, 2017

Uh oh!

shana commented Aug 29, 2017

Uh oh!

shana commented Aug 29, 2017

Uh oh!

shana left a comment

Choose a reason for hiding this comment

Uh oh!

StanleyGoldman commented Aug 30, 2017

StanleyGoldman commented Aug 21, 2017 •

edited

Loading

MunchyYDL commented Aug 23, 2017 •

edited

Loading

StanleyGoldman commented Aug 28, 2017 •

edited

Loading