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

Fix logic bugs related to e1e9427 #1398

Merged
merged 1 commit into from
Oct 27, 2017

Conversation

ewpatton
Copy link
Contributor

Thanks for submitting code to Blockly! Please fill out the following as part of your pull request so we can review your code more easily.

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

What Github issue does this resolve (please include link)?

Fixes #1373

Proposed Changes

Describe what this Pull Request does. Include screenshots if applicable.

This PR addresses #1373 by adding an else case in the event filter/merge mechanism to add events that collide with an existing hash but don't have paths for merging with existing events still get added to the event queue.

There is also a minor fix for an issue found with mutators where multiple move events are created and merged such that the resulting events are null, but still end up in the event queue and therefore the undo/redo stack. This causes UX problems because one may have to undo/redo twice.

Reason for Changes

Explain why these changes should be made. Include screenshots if applicable.

This commit is made to address #1373 where events that collided based on the hashing algorithm resulted in events being dropped.

Test Coverage

Please show how you have added tests to cover your changes, or tell us how you tested it and on which platforms. If it's plaform-agnostic you can delete these checkboxes.

Two tests were added to tests/jsunit/event_tests.js to test to ensure that the Scratch stackclick event doesn't get dropped following a click event and another to test that null events caused by merging are dropped.

Tested on:

  • Desktop:

    • Chrome
    • Firefox
    • Safari
    • Opera
    • IE 10+
    • IE 11
    • EDGE
  • Smartphone/Tablet/Chromebook (please complete the following information):

    • Device: [e.g. iPhone6]
    • OS: [e.g. iOS8.1]
    • Browser [e.g. stock browser, safari]
    • Version [e.g. 22]

Additional Information

Anything else we should know?

This commit adds a fallback when attempting to merge events that
collide on a hash but do not actually merge. Previously, the latter
event would be dropped.

This also fixes a minor UX problem where moving the isNull check into
the merging loop resulted in extra events in the undo queue that
served no purpose and required multiple undo operations without visual
feedback.

Fixes google#1373
@rachel-fenichel
Copy link
Collaborator

Thanks.

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