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

Fire EditorSelectionChanged on next button click #187

Merged
merged 1 commit into from Jun 7, 2018
Merged

Fire EditorSelectionChanged on next button click #187

merged 1 commit into from Jun 7, 2018

Conversation

izdwuut
Copy link
Contributor

@izdwuut izdwuut commented May 24, 2018

This is a solution that I propose for #1690. I consider it to be a suggestion, as building an installer was beyond me, thus I'm not sure if it would fix the issue.

As far as my comprehension goes, an EditorSelectionChanged procedure is responsible for toggling the "next" button. I noticed that it gets invoked in at least 2 situations:

  • when an user picks a text editor (an OnChange event)
  • once the custom page for configuring the default Git editor is initialized (the InitializeWizard procedure. I have just removed it, as I see it as redundant now)

My reasoning tells me that it would be applicable in the NextButtonClick procedure as well. I added an appropriate code snippet to it.

@izdwuut
Copy link
Contributor Author

izdwuut commented May 24, 2018

Ouch, I didn't add the sign-off. Is there a way that would allow me to append it?

@PhilipOakley
Copy link
Contributor

Simply amend your last commit, (or rebase for a series), and then force push the update to the same branch here at GitHub, and it will be detected and the new checks will be repeated, with hopeful success!

@izdwuut
Copy link
Contributor Author

izdwuut commented May 24, 2018

Works like a charm. Thanks for your assistance!

@dscho
Copy link
Member

dscho commented May 25, 2018

This looks pretty good already!

I would like to ask for two changes:

  • the description here in the PR is so nice, but the commit message is so empty. Could you paste the description into the commit message? When I work with code, I like to stay in my environment rather than hunt down information on the web, so reading an almost empty commit message when trying to figure out why a certain change was made is very frustrating.

  • could you also add a line This fixes ... with the URL to the issue it fixes? Then that ticket will auto-close when I merge the PR and we have a record in the commit message.

Thanks!

This fixes [#1690](git-for-windows/git#1690).

As far as my comprehension goes, an [EditorSelectionChanged](https://github.com/izdwuut/build-extra/blob/1d9630e321351b5ecd1609a3effd8df34cd948a1/installer/install.iss#L1084) procedure is responsible for toggling the "next" button. I noticed that it gets invoked in at least 2 situations:
  - when an user picks a text editor ([an OnChange event](https://github.com/izdwuut/build-extra/blob/1d9630e321351b5ecd1609a3effd8df34cd948a1/installer/install.iss#L1119))
  - once the custom page for configuring the default Git editor is initialized (the InitializeWizard procedure. I have just removed it, as I see it as redundant now)

My reasoning tells me that it would be applicable in the [NextButtonClick](https://github.com/izdwuut/build-extra/blob/1d9630e321351b5ecd1609a3effd8df34cd948a1/installer/install.iss#L1507) procedure as well. I added an appropriate code snippet to it.

Signed-off-by: Bartosz "izdwuut" Konikiewicz <izdwuut@gmail.com>
@izdwuut
Copy link
Contributor Author

izdwuut commented May 25, 2018

Thanks for prompting me to improve this PR! I had some second thoughts while composing it, therefore I'm glad for your guidance. Your wholehearted support assures me that I'm going to do just great the next time.

I decided to interchange the first paragraph with the line that you proposed, and pasted the whole thing into the commit description. Phew, what a journey!

@dscho
Copy link
Member

dscho commented Jun 7, 2018

Thank you, and sorry for the delay (I am still recovering from the work that went into v2.17.1 and v2.17.1(2))!

@dscho dscho merged commit abc8bfc into git-for-windows:master Jun 7, 2018
@izdwuut
Copy link
Contributor Author

izdwuut commented Jun 7, 2018

That's alright. I'm happy that I was able to provide some help. I'd say that you've merged my PR in the most auspicious time. I almost forgot that I proposed it, thus your confirmation came as a surprise to me. Thanks for assisting me in the process and I wish you all the best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants