Extend IJsDialogHandler To Include OnBeforeUnload Dialogs #715
Conversation
Extended IJsDialogHandler to catch on exit Javascript Dialog messages.
Added is_Reloaded
|
Not Sure how to update a pull request so I made a new one. Hope that was ok. It worries me that this failed appVeyor again. |
You just push more commits to the same branch, no need for a new PR.
All |
|
@daveaton It looks like your pretty close. I have a few questions, I'll comment inline. |
| handled = handler->OnJSBeforeUnload(_browserControl, StringUtils::ToClr(message_text), is_reload, resultString); | ||
| if (handled) | ||
| { | ||
| callback->Continue(is_reload, StringUtils::ToNative(resultString)); |
There was a problem hiding this comment.
Should is_reload be passed as first param here?
|
Hum... just learning how Git Hub works, it seems that it automatically pulls in changes, I thought I had to update the pull request some how.. but if this is the case doing a 1 to 1 master is not a good idea.. I should have created a branch and let you guys apply to master here... or am I wrong ? If I make changes later on will it not update this pull request automatically ? |
|
Any change you push to a branch is included into a PR up until the point of it being merged. Long as you don't need to make other changes on your |
| @@ -98,6 +98,7 @@ | |||
| <s:Boolean x:Key="/Default/Environment/SearchAndNavigation/GotoSingleUsageImmediately/@EntryValue">False</s:Boolean> | |||
| <s:Boolean x:Key="/Default/Environment/SearchAndNavigation/MergeOccurences/@EntryValue">True</s:Boolean> | |||
| <s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EMigrateBlankLinesAroundFieldToBlankLinesAroundProperty/@EntryIndexedValue">True</s:Boolean> | |||
| <s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EMigrateThisQualifierSettings/@EntryIndexedValue">True</s:Boolean> | |||
There was a problem hiding this comment.
Can you revert this change please? I'm guessing this was just an oversight.
This reverts commit 74a2f1a.
|
@daveaton I see you made the changes 👍 I'll take this for a spin ASAP 😄 (When you push more commits github doesn't notify me, so as a future reference if you could post a comment when changes are complete that would be much appreciated). |
|
@amaitland Sorry about that I did not want to seem pushy. I normally check stats about 1-3 times a day. I figured you were hard at work on v39 :) Now that I know I will start leaving comments. |
|
@daveaton Just a quick, |
|
@amaitland Will do :) |
|
@daveaton I merged this, made a few additional changes. You can review them here fce5f06 Basically I removed a few unnessicary params (The Js callback in I've tested with some basic scenarios, everything appears to be working. If you've got any comments please let me know 😄 |
Extended IJsDialogHandler to catch on OnBeforeUnload Javascript Dialog messages.
This should fix the issue with #712