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

AB-12 Added strokeprop event to fix replay stroke style bug #17

Merged
merged 8 commits into from
Aug 10, 2020

Conversation

stianlaa
Copy link
Collaborator

It seemed to me that some of the neccessary preparation for having strokeprops handled as an event was already done, and it appears no work well.

First time touching Drawify, so its likely great room for improvement 🐤

@holwech
Copy link
Owner

holwech commented May 31, 2020

I think this isn't completely right for some reasons I will try to explain. Just to define some things first that can be a little confusing for anyone who hasn't worked on this code before:

IEvent: Events do not by itself have any inherit meaning and so IEvent always have to be translated to an IAction based on the state of the Dispatcher. A general rule is that most events will come from Javascript event listeners.
IAction: Actions are the objects that get stored and replayed at a later point. They are basically the "frames" of the recording. Thinking about this, IAction should probably be renamed to IFrame or something like that as it's more descriptive. An IAction should be independent of all other IActions recorded (which is not really the case at the moment). This is why we want stroke props included in the IAction object, since right now IAction depend on the stroke props state in BoardController.
IUserAction: IUserAction modifies the board states that do not change how IActions are created. For example starting, pausing, restarting, etc. This is where stroke properties are placed now, but should be moved out of.
IModifier: IModifier modifies state changes that affect how IActions are created, for example PAN_ON/PAN_OFF. A mouse move IEvent gets converted to either a stroke or pan IAction, depending on whether pan is on or off. This is where I think stroke props probably belong, since the stroke props have to be applied to the IAction before they get stored by the Dispatcher.

Hope that wasn't too confusing...

@holwech
Copy link
Owner

holwech commented May 31, 2020

So basically:

  • Take stroke props state out of BoardController and move to Dispatcher
  • Make changes so that stroke props are added directly to the IAction and modify the BoardController to read those stroke props from the IAction
  • Change Service/Dispatcher so that stroke props are IModifier objects and that they modify the Dispatcher state

Or if you have any good ideas on how to do it in an even better way, feel free to share your suggestions :)

@stianlaa
Copy link
Collaborator Author

stianlaa commented Jun 8, 2020

I will take your reviews and comments into account and update the PR, I had a feeling I had made a significant structural misunderstanding, and I think your comments should go a long way to helping correct it.

I'm sure I'll also have some more detailed questions as I go about realizing the change.

Thanks for the feedback 👍 will fix when I find the time

@stianlaa
Copy link
Collaborator Author

stianlaa commented Jun 27, 2020

The recent commit is not yet ready for merge. I pushed it to fish for some feedback.

I'm having issues with replaying seemingly overwriting the strokeProps assigned to Actions, and can't exactly see where or why this should occur.

But I am beginning to understand the structure a lot better, so there's that :D

@holwech
Copy link
Owner

holwech commented Jun 28, 2020

This looks good! You're definitely on the right track now :) It looks like the example application is modifying values directly in the strokeProps object rather than creating a new copy on change. This results in all actions pointing to the same strokeProps object, which I'm guessing is the cause of the issue you're having.

@holwech
Copy link
Owner

holwech commented Jun 28, 2020

And if you can add a couple of tests, that would great (might be harder than I think it is...)

@stianlaa
Copy link
Collaborator Author

stianlaa commented Jul 4, 2020

Your recommended adjustments worked perfectly, nice! Manually testing shows decent behaviour.

Next I'll try writing some tests, although I expect this to be relatively hard.

@holwech
Copy link
Owner

holwech commented Jul 6, 2020

Looks good :)

@stianlaa stianlaa merged commit 6e9be00 into master Aug 10, 2020
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