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

Storing result of not filtered actions #86

Closed
peteruithoven opened this issue Apr 12, 2016 · 8 comments
Closed

Storing result of not filtered actions #86

peteruithoven opened this issue Apr 12, 2016 · 8 comments
Labels
bug Something in this project needs to be fixed

Comments

@peteruithoven
Copy link
Collaborator

Hi guys,
There is an aspect of the undo filtering I'm having a hard time with.
Currently, on each (not filtered) action we store the current state, before it's affected by the action. This mean we can also get the result of actions we're trying to filter away.

For example, I'm making a drawing app where users can select shapes by dragging a selection box. I'd like to keep this selection box from my undo history, so I'm ignoring the DRAGGING_SELECTION_BOX actions. But I do want to store the selection that is made, when the user releases it's mouse button. The steps:

  1. action: INIT
    undo: state is stored
  2. action: ADD_SHAPE
    undo: not storing state because DRAGGING_SELECTION_BOX is ignored
  3. action: DRAGGING_SELECTION_BOX (ignored)
    undo: state is stored (including unwanted selection box)
  4. action: SELECT

But then, when I undo, I'll get the state with the selection box.

I have the same problem with opened dropdown menus I'd like to keep from the undo history.

Shouldn't we store the result of the actions that we're not ignoring? Shouldn't we store the state after it's affected by the action? One complicating factor, I'm sure I'm forgetting other, is that the last past object can easily equal the present.

@omnidan omnidan added help wanted Help needed from the open source community question A question about the future of the project labels Apr 21, 2016
@peteruithoven
Copy link
Collaborator Author

I've made an example to clarify my point:
https://github.com/peteruithoven/redux-undo-filtering-issue-example

@omnidan
Copy link
Owner

omnidan commented Apr 29, 2016

Sorry it took me so long to get to this. Thank you for your detailed write-up and example - great work 😁

I managed to reproduce this issue and I don't think the default behaviour should be like that. In 382a5ce, I adjusted the insertion behaviour to store to past on the next action that happens (and if the previous action was filtered, I tag the history with wasFiltered: true and then don't store on the next action).

I hope this fixes your problems and also avoids confusion in the future. 😉

@omnidan omnidan added bug Something in this project needs to be fixed and removed help wanted Help needed from the open source community question A question about the future of the project labels Apr 29, 2016
omnidan added a commit that referenced this issue Apr 30, 2016
@omnidan
Copy link
Owner

omnidan commented May 3, 2016

Fixed in beta8

@joshkel
Copy link

joshkel commented May 24, 2016

I'm trying to make sure I understand this correctly.

If you had a series of actions, A, b, c, D, e, f, where uppercase actions are tracked and lowercase actions are ignored:

  • Prior to this change (i.e., prior to beta8), an undo operation would go to just before D: it would put the state back to what it was just before the most recent tracked change.
  • After this change, an undo operation would go to just after A: it would put the state back to what it was before the most recent change as well as undoing any filtered actions before that change.

Is this correct?

My usage scenario is the opposite of @peteruithoven . I'm integrating with redux-form, so the actions are a sequence of form field events (initialize form, change active tab, focus field, change field, blur field, etc.). I'm filtering out everything except "change field" (because undoing a focus or blur operation would be silly), but I want to restore to the exact state at the time of change (i.e., if I undo a change to a field, I want that field's tab to be active and that field to be focused).

Could redux-undo be extended to support both scenarios? Maybe via a trackFilteredStates boolean parameter, or maybe by offering separate filterFromHistory and filterFromState functions (better names needed)? I can submit a PR if you're interested.

@peteruithoven
Copy link
Collaborator Author

Since we're not really undoing actions but reverting to older states I would describe the "After this change," part differently, more like

After this change, an undo operation would go to just after A (the state after the last action that wasn't filtered).

I would assume that if you go back to a state where a field was changed, that state also has that field in focus. But then again, I'm not even sure focus is part of the state with redux-form.

Could you add an simple example?

@joshkel
Copy link

joshkel commented May 24, 2016

Sure. Here's how we're using redux-form, simplified. redux-undo is configured to ignore every action except for CHANGE (which are bolded below).

  1. Initial form state:

    { _active: 'firstName', fields: {
        firstName: { value: 'John' },
        lastName: { value: 'Doe' }
    }}
  2. Change first name. Action: { type: 'redux-form/CHANGE', field: 'firstName', value, 'Bob'}

    { _active: 'firstName', fields: {
        firstName: { value: 'Bob' },
        lastName: { value: 'Doe' }
    }}
  3. Click on last name. Action: { type: 'redux-form/BLUR', field: 'firstName'}

    { _active: undefined, fields: {
        firstName: { value: 'Bob' },
        lastName: { value: 'Doe' }
    }}
  4. Click on last name (continued). Action: { type: 'redux-form/FOCUS', field: 'lastName'}

    { _active: 'lastName', fields: {
        firstName: { value: 'Bob' },
        lastName: { value: 'Doe' }
    }}
  5. Change last name. Action: { type: 'redux-form/CHANGE', field: 'lastName', value: 'Smith'}

    { _active: 'lastName', fields: {
        firstName: { value: 'Bob' },
        lastName: { value: 'Smith' }
    }}
  6. Leave the form. Action: { type: 'redux-form/BLUR', field: 'lastName'}

    { _active: undefined, fields: {
        firstName: { value: 'Bob' },
        lastName: { value: 'Doe' }
    }}

If I perform an undo at this point, beta7 does what we want: action 6 was ignored, so it restores the state to just before action 5. lastName's value is Doe (which is what we want), and the active field is lastName (which is nice, because it shows the user what they just undid).

If I understand this change correctly, then if I perform an undo at this point, it will restore the state to the most recent non-ignored state before action 5 - i.e., it restores to the state just after action 2. lastName will be Doe (which is what we want), but the active field is firstName (which seems like it would be potentially confusing).

@peteruithoven
Copy link
Collaborator Author

I understand the case. I understand that that would be very confusing, but I also think the change makes sense. It looks like you'd like to also store the state after the latest focus action, but then forget about the previous focus states.
The contributors probably have better ideas, but one idea might be to have a way to filter the past after a history change. You might be able to currently do this with a higher level reducer around the created undoable reducer, this could filter the history after each action.

It would be really helpful though if you could make a technical example, it really helped out Omnidan understanding and fixing my issue.

@peteruithoven
Copy link
Collaborator Author

I tried to visualize your usecase. White dots are ignored states, black dots are stored states. The green is what you'd like to see happen, the red is what currently happens.
redux-form-undo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something in this project needs to be fixed
Projects
None yet
Development

No branches or pull requests

3 participants