Skip to content
This repository has been archived by the owner on Aug 31, 2021. It is now read-only.

[[ Bug 16032 ]] gradientrampeditor widget does not catch mouse relea… #2930

Merged
merged 3 commits into from
Sep 29, 2015

Conversation

BerndN
Copy link
Contributor

@BerndN BerndN commented Sep 27, 2015

…se outside card bounds

gradientrampeditor does not catch mouseUp when it occurs outside of card bounds. Added OnMouseCancel handler.

…e outside card bounds

gradientrampeditor does not catch mouseUp when it occurs outside of card bounds. Added OnMouseCancel handler.
@livecode-vulcan
Copy link
Contributor

📝 Hi @BerndN, I haven't been able to verify that you've signed our LiveCode Contributor's Agreement.

If you have previously signed the Contributor's Agreement, I may not be able to detect it because you haven't linked your GitHub account to your LiveCode account.

Please see the information for contributors for more information.

@BerndN
Copy link
Contributor Author

BerndN commented Sep 27, 2015

Linked Livecode to my account
Bernd

@BerndN
Copy link
Contributor Author

BerndN commented Sep 28, 2015

Mark Waddingham commented on the forum that actually OnMouseUp should work wherever the mouse goes up
http://forums.livecode.com/viewtopic.php?f=93&t=25414&p=132114#p132110
So this pull request might be not necessary when the intended behavior is instated

@runrevmark
Copy link
Contributor

@BerndN: Whilst there is a bug in the widget event flow (from what I can see), the mouseCancel handler you've added is still necessary... There's nothing to stop a script from causing UI context to change whilst a drag is occurring in the gradient ramp editor (a script could decide to popup a dialog box randomly, for example) so your change is still needed :)

@BerndN
Copy link
Contributor Author

BerndN commented Sep 28, 2015

beginners luck :)

@@ -285,6 +285,10 @@ public handler OnMouseDoubleUp() returns nothing
gradientStopsChanged()
end handler

public handler onMouseCancel() returns nothing
put false into mIsDraggingStop
Copy link
Contributor

Choose a reason for hiding this comment

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

The mouseCancel handler should function in the same way as mouseUp, by making sure that we redraw and send the gradient stop changed message.

put xCoordToStopNumber(the x of the mouse position) into mCurrentlySelectedStopIndex
redraw all
gradientStopsChanged()

It might be worth refactoring the common code in mouseUp and mouseCancel into a new handler.

@livecodemichael
Copy link
Contributor

Hi @BerndN

Thanks very much for the pull request. I've added an inline comment on a minor update.

On more thing we require with bug fixes is an additional file called
bugfix-.md
in the notes directory, which contains a one-line description of the bug fixed in the form

Could you also add this file to the pull request please?

@BerndN
Copy link
Contributor Author

BerndN commented Sep 28, 2015

@livecodemichael
do you want me to retract the current pull request and do a new one with your suggestion of replicating the OnMouseUp behavior (which makes a lot of sense and works, too)?

@peter-b peter-b mentioned this pull request Sep 28, 2015
@livecodemichael
Copy link
Contributor

@BerndN

Using this current pull request is fine. Any commits you make to your branch patch-1 will be reflected here.

Your update to the onMouseCancel looks good. Thanks for that.

All that remains is to add the bug fix note. Add a file docs/notes/bugfix-16032.md to you branch patch-1 with the content # catch OnMouseCancel in gradientrampeditor and it should appear in this pull request.

Once that's done we can get it reviewed and integrated.

Thanks very much.

@BerndN
Copy link
Contributor Author

BerndN commented Sep 29, 2015

Sorry, I thought I had done it but only now saw "patch-1" for some reason and tried again.
Thank you for your patience.

@peter-b
Copy link
Contributor

peter-b commented Sep 29, 2015

@BerndN Thank you! I'm glad you managed to work it out in the end.

For next time, please bear in mind that in general we would like all commits to have clear & descriptive commit messages. A good way to think about a good commit message is that the first line should complete the sentence, "When merged, this commit will...". For example:

When merged, this commit will Add a release note for the gradientrampeditor widget

For more information, I recommend this article: How to write a commit message

@livecodemichael
Copy link
Contributor

@livecode-vulcan review ok 725fc07

@livecode-vulcan
Copy link
Contributor

💙 review by @livecodemichael ok 725fc07

livecode-vulcan added a commit that referenced this pull request Sep 29, 2015
[[ Bug 16032 ]]  gradientrampeditor widget does not catch mouse relea…

…se outside card bounds

gradientrampeditor does not catch mouseUp when it occurs outside of card bounds. Added OnMouseCancel handler.
@livecode-vulcan
Copy link
Contributor

😎 test success 725fc07

@livecodeali livecodeali added this to the 8.0.0-dp-6 milestone Sep 29, 2015
livecodeali added a commit that referenced this pull request Sep 29, 2015
[[ Bug 16032 ]]  gradientrampeditor widget does not catch mouse relea…
@livecodeali livecodeali merged commit d822f02 into livecode:develop Sep 29, 2015
@BerndN BerndN deleted the patch-1 branch September 29, 2015 13:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants