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

[FEATURE] adds undo and redo on transaction groups #4765

Merged
merged 2 commits into from Sep 15, 2017

Conversation

Projects
None yet
4 participants
@vmora
Contributor

vmora commented Jun 22, 2017

This work is related to qgis/QGIS-Enhancement-Proposals#97

Only the AddFeature function has been coded for now in order to get some feedback on the chosen direction.

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Jun 27, 2017

Contributor

@m-kuhn does that sound a good start to have the undo redo in QGIS transactions?

Contributor

haubourg commented Jun 27, 2017

@m-kuhn does that sound a good start to have the undo redo in QGIS transactions?

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jun 27, 2017

Member

Yes, this looks very promising, the direction with savepoints sounds very good to me.

Some considerations:

  • Exceptions are not very much used throughout QGIS and Qt. Some methods that throw exceptions are overridden Qt methods. Have you verified that all the codepaths that can call these methods safely handle exceptions (without leaking etc)?
  • It might be worthwile to add new virtual methods QgsTransaction::createSavepoint() that creates a new savepoint (or returns the current one if nothing happened since the last savepoint?), QgsTransaction::currentSavepoint() (and possibly bool QgsTransaction::currentSavepointValid() that returns false if an operation took place since the last savepoint)
  • Out of curiosity, how heavy are savepoints (memory- and cpu-wise)?
  • I think we can directly rollback a couple of undo commands in a single step(?). I don't know the performance impact of a single rollback, might be worth detecting such a "fast rollback" and skipping intermediate savepoints, not sure.
  • Use QgsDebugMsg instead of std::cout (consistency with the rest of the code)
  • Setup your pre-commit hook
Member

m-kuhn commented Jun 27, 2017

Yes, this looks very promising, the direction with savepoints sounds very good to me.

Some considerations:

  • Exceptions are not very much used throughout QGIS and Qt. Some methods that throw exceptions are overridden Qt methods. Have you verified that all the codepaths that can call these methods safely handle exceptions (without leaking etc)?
  • It might be worthwile to add new virtual methods QgsTransaction::createSavepoint() that creates a new savepoint (or returns the current one if nothing happened since the last savepoint?), QgsTransaction::currentSavepoint() (and possibly bool QgsTransaction::currentSavepointValid() that returns false if an operation took place since the last savepoint)
  • Out of curiosity, how heavy are savepoints (memory- and cpu-wise)?
  • I think we can directly rollback a couple of undo commands in a single step(?). I don't know the performance impact of a single rollback, might be worth detecting such a "fast rollback" and skipping intermediate savepoints, not sure.
  • Use QgsDebugMsg instead of std::cout (consistency with the rest of the code)
  • Setup your pre-commit hook
@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Jun 28, 2017

Contributor

For the only part I can answer:

Out of curiosity, how heavy are savepoints (memory- and cpu-wise)?
It seems to be like a transaction size.
https://serverfault.com/questions/214834/how-much-memory-does-one-postgresql-savepoint-take-up

When saving or exiting edit mode we need to be sure to emit a Release SavePoint . Googling a bit, I found issues on memory consumption, all are very old, and edge cases (JDBC using Latin1 encoding)
see https://www.spinics.net/lists/pgsql/msg93522.html

@vmora, I let you answering the other good questions :-)

Contributor

haubourg commented Jun 28, 2017

For the only part I can answer:

Out of curiosity, how heavy are savepoints (memory- and cpu-wise)?
It seems to be like a transaction size.
https://serverfault.com/questions/214834/how-much-memory-does-one-postgresql-savepoint-take-up

When saving or exiting edit mode we need to be sure to emit a Release SavePoint . Googling a bit, I found issues on memory consumption, all are very old, and edge cases (JDBC using Latin1 encoding)
see https://www.spinics.net/lists/pgsql/msg93522.html

@vmora, I let you answering the other good questions :-)

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Jul 3, 2017

Contributor

@m-kuhn Thanks for the review. Answers follow, but, to make it short, the crucial issue for me is to find a way to handle errors in undo/redo commands. If anyone as an idea, I'll be more than happy to implement/test it.

Exceptions are not very much used throughout QGIS and Qt. Some methods that throw exceptions are overridden Qt methods.

I'm fully conscious of the fact, and not especially happy with the design. The problem is: the undo/redo is done by the Qt undo-stack, and the redo function has no error mechanism that I could find, while the provider can return an error... hence I resorted to exception mechanism.

Have you verified that all the codepaths that can call these methods safely handle exceptions (without leaking etc)?

Nope, and I deserve a good ol' flogging for that. The undo (which raises) is connected to a signal, I suppose an uncaught exception there will be trouble, but I'm not sure (http://www.qtcentre.org/archive/index.php/t-44741.html). Under normal circumstances, only the first redo (called by the push) should throw... but if the connection is lost or whatever...

If someone has a clue as to how to handle errors cleanly in a redo, please !

Is the absence of featureAdded signal sufficient ?

It might be worthwile to add new virtual methods QgsTransaction::createSavepoint() that creates a new savepoint (or returns the current one if nothing happened since the last savepoint?), QgsTransaction::currentSavepoint() (and possibly bool QgsTransaction::currentSavepointValid() that returns false if an operation took place since the last savepoint)

Good idea.

I think we can directly rollback a couple of undo commands in a single step(?). I don't know the performance impact of a single rollback, might be worth detecting such a "fast rollback" and skipping intermediate savepoints, not sure.

I don't understand how the "fast roolback" happens on the user side. If you are talking about the insertion of several features (e.g. cpy/paste), then it's a mater of not implementing addFeatures as several calls to addFeature. Or are you talking about something else entirely ?

Use QgsDebugMsg instead of std::cout (consistency with the rest of the code)

Will do my best, but old habits die hard.

Setup your pre-commit hook

Done. Thanks for the reminder.

Contributor

vmora commented Jul 3, 2017

@m-kuhn Thanks for the review. Answers follow, but, to make it short, the crucial issue for me is to find a way to handle errors in undo/redo commands. If anyone as an idea, I'll be more than happy to implement/test it.

Exceptions are not very much used throughout QGIS and Qt. Some methods that throw exceptions are overridden Qt methods.

I'm fully conscious of the fact, and not especially happy with the design. The problem is: the undo/redo is done by the Qt undo-stack, and the redo function has no error mechanism that I could find, while the provider can return an error... hence I resorted to exception mechanism.

Have you verified that all the codepaths that can call these methods safely handle exceptions (without leaking etc)?

Nope, and I deserve a good ol' flogging for that. The undo (which raises) is connected to a signal, I suppose an uncaught exception there will be trouble, but I'm not sure (http://www.qtcentre.org/archive/index.php/t-44741.html). Under normal circumstances, only the first redo (called by the push) should throw... but if the connection is lost or whatever...

If someone has a clue as to how to handle errors cleanly in a redo, please !

Is the absence of featureAdded signal sufficient ?

It might be worthwile to add new virtual methods QgsTransaction::createSavepoint() that creates a new savepoint (or returns the current one if nothing happened since the last savepoint?), QgsTransaction::currentSavepoint() (and possibly bool QgsTransaction::currentSavepointValid() that returns false if an operation took place since the last savepoint)

Good idea.

I think we can directly rollback a couple of undo commands in a single step(?). I don't know the performance impact of a single rollback, might be worth detecting such a "fast rollback" and skipping intermediate savepoints, not sure.

I don't understand how the "fast roolback" happens on the user side. If you are talking about the insertion of several features (e.g. cpy/paste), then it's a mater of not implementing addFeatures as several calls to addFeature. Or are you talking about something else entirely ?

Use QgsDebugMsg instead of std::cout (consistency with the rest of the code)

Will do my best, but old habits die hard.

Setup your pre-commit hook

Done. Thanks for the reminder.

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jul 3, 2017

Member

If someone has a clue as to how to handle errors cleanly in a redo, please !

In the constructor you receive a pointer to the edit buffer, could you use that to send an error-signal (to at least show it to the user)?
Not sure how to remove the redo command from the stack in this case, but maybe you could somehow "flag" it as failed and leave it on the stack.

Is the absence of featureAdded signal sufficient ?

I'd say that shouldn't be sent in this case at least, yes. Anything more than that would be nice ot have. @wonder-sk what do you think?

I don't understand how the "fast roolback" happens on the user side.

I thought I've seen somewhere a "history" list where a user could directly click 7 steps back, but not sure and this also is just "nice to have" in case 7 savepoint rollbacks are (considerably) slower than a single one.

Member

m-kuhn commented Jul 3, 2017

If someone has a clue as to how to handle errors cleanly in a redo, please !

In the constructor you receive a pointer to the edit buffer, could you use that to send an error-signal (to at least show it to the user)?
Not sure how to remove the redo command from the stack in this case, but maybe you could somehow "flag" it as failed and leave it on the stack.

Is the absence of featureAdded signal sufficient ?

I'd say that shouldn't be sent in this case at least, yes. Anything more than that would be nice ot have. @wonder-sk what do you think?

I don't understand how the "fast roolback" happens on the user side.

I thought I've seen somewhere a "history" list where a user could directly click 7 steps back, but not sure and this also is just "nice to have" in case 7 savepoint rollbacks are (considerably) slower than a single one.

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Jul 7, 2017

Contributor

@m-kuhn apparently QUndoCommand::setObsolete should do the trick for command removal, but it's only available since Qt 5.9.

I did start to implement QgsTransaction::createSavepoint()... but revreted. I think it too much hassle for a virtual class that has only one derived class (postgres). If SAVEPOINT or ROOLBACK TO SAVEPOINT which are standard SQL, are not supported by the next derived class then we may do something more complicated, until then I prefer to keep things minimal.

I removed the use of exceptions, and all should be fine and safe now.

I also changed the behavior of the editPaste function, to use addFeatures instead of addFeature, except for a bigger memory footprint, it should be an improvement overall.

Contributor

vmora commented Jul 7, 2017

@m-kuhn apparently QUndoCommand::setObsolete should do the trick for command removal, but it's only available since Qt 5.9.

I did start to implement QgsTransaction::createSavepoint()... but revreted. I think it too much hassle for a virtual class that has only one derived class (postgres). If SAVEPOINT or ROOLBACK TO SAVEPOINT which are standard SQL, are not supported by the next derived class then we may do something more complicated, until then I prefer to keep things minimal.

I removed the use of exceptions, and all should be fine and safe now.

I also changed the behavior of the editPaste function, to use addFeatures instead of addFeature, except for a bigger memory footprint, it should be an improvement overall.

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Jul 7, 2017

Contributor

@haubourg could you give it a try in a real-world case ?

Contributor

vmora commented Jul 7, 2017

@haubourg could you give it a try in a real-world case ?

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jul 7, 2017

Member

Sounds great, @vmora

What's the hassle with QgsTransaction::createSavepoint()? I was really looking forward to that as I had some ideas what to do with the knowledge of the "current" savepoint.

Member

m-kuhn commented Jul 7, 2017

Sounds great, @vmora

What's the hassle with QgsTransaction::createSavepoint()? I was really looking forward to that as I had some ideas what to do with the knowledge of the "current" savepoint.

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Jul 10, 2017

Contributor

What's the hassle with QgsTransaction::createSavepoint()

  • Should it be pure virtual or do nothing by default ? Or shall I implement directly in the base class ?
  • Is there the need for a bool QgsTransaction::supportsSavepoint() ? I hope not (I like small interfaces a better).
  • We need a QgsTransaction::roolbackToSavepoint(savepoint), does it remove the savepoint, or do we also want a QgsTransaction::removeSavepoint(savepoint) ? I believe the former is better, in order to write `tr->rollbackToSavepoint( tr->currentSavepoint() )
  • Having a currentSavepoint sorts of duplicate the state/functionality of the undostack, is it what you want ?

The coding is pretty straightforward though, so I can give it a another try and see what it looks like.

Contributor

vmora commented Jul 10, 2017

What's the hassle with QgsTransaction::createSavepoint()

  • Should it be pure virtual or do nothing by default ? Or shall I implement directly in the base class ?
  • Is there the need for a bool QgsTransaction::supportsSavepoint() ? I hope not (I like small interfaces a better).
  • We need a QgsTransaction::roolbackToSavepoint(savepoint), does it remove the savepoint, or do we also want a QgsTransaction::removeSavepoint(savepoint) ? I believe the former is better, in order to write `tr->rollbackToSavepoint( tr->currentSavepoint() )
  • Having a currentSavepoint sorts of duplicate the state/functionality of the undostack, is it what you want ?

The coding is pretty straightforward though, so I can give it a another try and see what it looks like.

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jul 10, 2017

Member

Should it be pure virtual or do nothing by default ? Or shall I implement directly in the base class ?

you say it's standard sql? do it in the base class and let the next dev who integrates another provider decide.

Is there the need for a bool QgsTransaction::supportsSavepoint() ? I hope not (I like small interfaces a better).

dito. let's do this only when required. if ever.

We need a QgsTransaction::roolbackToSavepoint(savepoint), does it remove the savepoint, or do we also want a QgsTransaction::removeSavepoint(savepoint) ? I believe the former is better, in order to write `tr->rollbackToSavepoint( tr->currentSavepoint() )

not sure. probably leave it and also intermediate ones for redo? unreachable intermediate ones could be removed once a new SP is created on top of an existing one?

Having a currentSavepoint sorts of duplicate the state/functionality of the undostack, is it what you want ?

I would like to have a "handle" of the current state for other parallel (r/o) connections.

The coding is pretty straightforward though, so I can give it a another try and see what it looks like.

👍

Member

m-kuhn commented Jul 10, 2017

Should it be pure virtual or do nothing by default ? Or shall I implement directly in the base class ?

you say it's standard sql? do it in the base class and let the next dev who integrates another provider decide.

Is there the need for a bool QgsTransaction::supportsSavepoint() ? I hope not (I like small interfaces a better).

dito. let's do this only when required. if ever.

We need a QgsTransaction::roolbackToSavepoint(savepoint), does it remove the savepoint, or do we also want a QgsTransaction::removeSavepoint(savepoint) ? I believe the former is better, in order to write `tr->rollbackToSavepoint( tr->currentSavepoint() )

not sure. probably leave it and also intermediate ones for redo? unreachable intermediate ones could be removed once a new SP is created on top of an existing one?

Having a currentSavepoint sorts of duplicate the state/functionality of the undostack, is it what you want ?

I would like to have a "handle" of the current state for other parallel (r/o) connections.

The coding is pretty straightforward though, so I can give it a another try and see what it looks like.

👍

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Jul 10, 2017

Contributor

@m-kuhn does this commit look like what you had in mind ?

Contributor

vmora commented Jul 10, 2017

@m-kuhn does this commit look like what you had in mind ?

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Jul 17, 2017

Contributor

@m-kuhn Are the more recent commits looking good to you? Can we proceed and extend to update and delete operations?

Contributor

haubourg commented Jul 17, 2017

@m-kuhn Are the more recent commits looking good to you? Can we proceed and extend to update and delete operations?

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jul 26, 2017

Member

@vmora what I have in mind requires the "current" state as a savepoint. So I imagine a savepoint to be created at the end of each edit command.

If I'm not mistaken, this pull request currently creates a savepoint before starting a new edit command (to backup previous work), is this correct?

Member

m-kuhn commented Jul 26, 2017

@vmora what I have in mind requires the "current" state as a savepoint. So I imagine a savepoint to be created at the end of each edit command.

If I'm not mistaken, this pull request currently creates a savepoint before starting a new edit command (to backup previous work), is this correct?

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Jul 28, 2017

Contributor

@m-kuhn you are right, the savepoint is before the edit command, otherwise it'd be a bit complicated to undo the first edit command.

The notion of a 'current' sate makes me think about an undo stack on the transaction side, is that what you have in mind ?

Contributor

vmora commented Jul 28, 2017

@m-kuhn you are right, the savepoint is before the edit command, otherwise it'd be a bit complicated to undo the first edit command.

The notion of a 'current' sate makes me think about an undo stack on the transaction side, is that what you have in mind ?

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jul 28, 2017

Member

The notion of a 'current' sate makes me think about an undo stack on the transaction side, is that what you have in mind ?

Not sure I understand this. My goal is just to create a second connection and "checkout" the "current" savepoint.

Member

m-kuhn commented Jul 28, 2017

The notion of a 'current' sate makes me think about an undo stack on the transaction side, is that what you have in mind ?

Not sure I understand this. My goal is just to create a second connection and "checkout" the "current" savepoint.

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Aug 3, 2017

Contributor

My goal is just to create a second connection and "checkout" the "current" savepoint

Can you really do that ? I can't find a reference for several connection to the same transaction.

Contributor

vmora commented Aug 3, 2017

My goal is just to create a second connection and "checkout" the "current" savepoint

Can you really do that ? I can't find a reference for several connection to the same transaction.

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Aug 3, 2017

Member

I don't know yet, but if it works it would be great and at least in theory it sounds feasible :)

Member

m-kuhn commented Aug 3, 2017

I don't know yet, but if it works it would be great and at least in theory it sounds feasible :)

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Aug 7, 2017

Contributor

@m-kuhn ok, I think I got it, I'll try and add save points at the end of the edit command, instead of the beginning.

I can always rollback to the beginning of the transaction to undo the first edit command. So their is always a current save point.

EDIT: nope, I can't rollback to the begining of the transaction, otherwise it's closed. So I must set a savepoint at the beginning and at the end, and use your 'if nothing happened' condition to avoid duplicating the savepoint. The problem is that the 'something happened' is in the derived QgsTransaction classes (i.e. executeSQL).

Contributor

vmora commented Aug 7, 2017

@m-kuhn ok, I think I got it, I'll try and add save points at the end of the edit command, instead of the beginning.

I can always rollback to the beginning of the transaction to undo the first edit command. So their is always a current save point.

EDIT: nope, I can't rollback to the begining of the transaction, otherwise it's closed. So I must set a savepoint at the beginning and at the end, and use your 'if nothing happened' condition to avoid duplicating the savepoint. The problem is that the 'something happened' is in the derived QgsTransaction classes (i.e. executeSQL).

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Aug 8, 2017

Contributor

@m-kuhn I improved (or I believe I did) the QgsTransaction interface:

    /**
     * creates a save point
     * returns empty string on error
     * returns the last created savepoint if it's not dirty
     */
    QString createSavepoint( QString &error SIP_OUT );

    /**
     * rollback to save point, the save point is maintained and is "undertied"
     */
    bool rollbackToSavepoint( const QString &name, QString &error SIP_OUT );

    /**
     * dirty save point such that next call to createSavepoint will create a new one
     */
    void dirtyLastSavePoint();

It'd be nice to call dirtyLastSavePoint() on connection commit(), but it doesn't keep et ref. to the transaction (just a bool to know if there is a transaction), so I dirty the last save point after each commit in the postgres provider.

I also removed the possibility to name a save point in the transaction interface 'cause it uselessly complexified the class. If you need a named save point you can always executeSQL.

I have not added a save point after the edit command, if you want me to, I can do that, or you can do that when/if you implement what you had in mind. With the new save point mechanism, it will only create the save point for the next edit command.

I'll implement the remaining undo/redo functions and remove the WIP tag asap.

Contributor

vmora commented Aug 8, 2017

@m-kuhn I improved (or I believe I did) the QgsTransaction interface:

    /**
     * creates a save point
     * returns empty string on error
     * returns the last created savepoint if it's not dirty
     */
    QString createSavepoint( QString &error SIP_OUT );

    /**
     * rollback to save point, the save point is maintained and is "undertied"
     */
    bool rollbackToSavepoint( const QString &name, QString &error SIP_OUT );

    /**
     * dirty save point such that next call to createSavepoint will create a new one
     */
    void dirtyLastSavePoint();

It'd be nice to call dirtyLastSavePoint() on connection commit(), but it doesn't keep et ref. to the transaction (just a bool to know if there is a transaction), so I dirty the last save point after each commit in the postgres provider.

I also removed the possibility to name a save point in the transaction interface 'cause it uselessly complexified the class. If you need a named save point you can always executeSQL.

I have not added a save point after the edit command, if you want me to, I can do that, or you can do that when/if you implement what you had in mind. With the new save point mechanism, it will only create the save point for the next edit command.

I'll implement the remaining undo/redo functions and remove the WIP tag asap.

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Aug 8, 2017

Contributor

@haubourg, @m-kuhn I have witnessed strange behavior for add/remove fields, maybe they should be "un-undoable" actions ;), what do you think ?

For bulk updates of fields, there is an undo command for each feature, I can see potential problems with that (lots of save points for large datasets).

Anyway, in the current state, you can test the branch and tell me what you think of it.

Contributor

vmora commented Aug 8, 2017

@haubourg, @m-kuhn I have witnessed strange behavior for add/remove fields, maybe they should be "un-undoable" actions ;), what do you think ?

For bulk updates of fields, there is an undo command for each feature, I can see potential problems with that (lots of save points for large datasets).

Anyway, in the current state, you can test the branch and tell me what you think of it.

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Aug 12, 2017

Member

I have witnessed strange behavior for add/remove fields, maybe they should be "un-undoable" actions ;), what do you think ?

It'd be better to make them behave nicely 😉

For bulk updates of fields, there is an undo command for each feature, I can see potential problems with that (lots of save points for large datasets).

They should be grouped with editCommandStarted and editCommandEnded.

I hope I find the time to give it a go, but not dure I find it actually.

Member

m-kuhn commented Aug 12, 2017

I have witnessed strange behavior for add/remove fields, maybe they should be "un-undoable" actions ;), what do you think ?

It'd be better to make them behave nicely 😉

For bulk updates of fields, there is an undo command for each feature, I can see potential problems with that (lots of save points for large datasets).

They should be grouped with editCommandStarted and editCommandEnded.

I hope I find the time to give it a go, but not dure I find it actually.

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Aug 25, 2017

Contributor

I build your branch and tested only now;

So, it works pretty well with a production project (not QGEP) for update / insert and delete.
I noticed the following :

  • if a transaction failed for any reason, a "action failed" is added to the undo/redo panel. I think we can filter those, since undoing them or redoing them makes no sense

  • Redoing a DELETE does not work. I don't see why it couldn't be replayed

  • Trying to play with data structure (which I do rarely from QGIS) triggers a direct crash when trying to add a field to a table. Since I have crashes, I can't reproduce your strange behavior !
    An unrelated thing, adding a field to a view does not work, QGIS throws an ALTER TABLE. statement. @m-kuhn I remember that in QGEP it worked (which was surprising and led to the action of adding new roles )

I also saw some weird traces in the stdout:

ERROR 4: /initrd.img: Permission non accordée
QInotifyFileSystemWatcherEngine::addPaths: inotify_add_watch failed: Aucun fichier ou dossier de ce type
Contributor

haubourg commented Aug 25, 2017

I build your branch and tested only now;

So, it works pretty well with a production project (not QGEP) for update / insert and delete.
I noticed the following :

  • if a transaction failed for any reason, a "action failed" is added to the undo/redo panel. I think we can filter those, since undoing them or redoing them makes no sense

  • Redoing a DELETE does not work. I don't see why it couldn't be replayed

  • Trying to play with data structure (which I do rarely from QGIS) triggers a direct crash when trying to add a field to a table. Since I have crashes, I can't reproduce your strange behavior !
    An unrelated thing, adding a field to a view does not work, QGIS throws an ALTER TABLE. statement. @m-kuhn I remember that in QGEP it worked (which was surprising and led to the action of adding new roles )

I also saw some weird traces in the stdout:

ERROR 4: /initrd.img: Permission non accordée
QInotifyFileSystemWatcherEngine::addPaths: inotify_add_watch failed: Aucun fichier ou dossier de ce type
@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Aug 28, 2017

Contributor

@haubourg

if a transaction failed for any reason, a "action failed" is added to the undo/redo panel. I think we can filter those, since undoing them or redoing them makes no sense

Well, it will be possible with Qt 5.9 ( see above: "apparently QUndoCommand::setObsolete should do the trick for command removal, but it's only available since Qt 5.9.")

Redoing a DELETE does not work. I don't see why it couldn't be replayed

Works for me (TM)... seriously, I'll look into that.

@m-kuhn

They should be grouped with editCommandStarted and editCommandEnded

I'll look into that.

Contributor

vmora commented Aug 28, 2017

@haubourg

if a transaction failed for any reason, a "action failed" is added to the undo/redo panel. I think we can filter those, since undoing them or redoing them makes no sense

Well, it will be possible with Qt 5.9 ( see above: "apparently QUndoCommand::setObsolete should do the trick for command removal, but it's only available since Qt 5.9.")

Redoing a DELETE does not work. I don't see why it couldn't be replayed

Works for me (TM)... seriously, I'll look into that.

@m-kuhn

They should be grouped with editCommandStarted and editCommandEnded

I'll look into that.

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Aug 28, 2017

Member

Well, it will be possible with Qt 5.9 ( see above: "apparently QUndoCommand::setObsolete should do the trick for command removal, but it's only available since Qt 5.9.")

Can this be #ifdef'd, then it will be available for everyone in a year or two.

Member

m-kuhn commented Aug 28, 2017

Well, it will be possible with Qt 5.9 ( see above: "apparently QUndoCommand::setObsolete should do the trick for command removal, but it's only available since Qt 5.9.")

Can this be #ifdef'd, then it will be available for everyone in a year or two.

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Aug 30, 2017

Contributor

@haubourg fixed problems with attr add/delete, normally everything works. Could you give it a try plz.

@m-kuhn thanks for the review, I'll have a look latter this afternoon.

Contributor

vmora commented Aug 30, 2017

@haubourg fixed problems with attr add/delete, normally everything works. Could you give it a try plz.

@m-kuhn thanks for the review, I'll have a look latter this afternoon.

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Aug 31, 2017

Contributor

@vmora tested this morning, it looks good to me, playing with structure changes and data changes seems stable.
I think you can proceed with PR's checklist (and add it in the PR description above) :

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit
Contributor

haubourg commented Aug 31, 2017

@vmora tested this morning, it looks good to me, playing with structure changes and data changes seems stable.
I think you can proceed with PR's checklist (and add it in the PR description above) :

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@vmora vmora changed the title from [WIP] adds undo and redo on transaction groups to [FEATURE] adds undo and redo on transaction groups Sep 4, 2017

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Sep 5, 2017

Contributor

@haubourg can you plz try it out once all green and give the go for merge ?

Contributor

vmora commented Sep 5, 2017

@haubourg can you plz try it out once all green and give the go for merge ?

[FEATURE] adds undo/redo for transaction groups
[needs-docs] the undo/redo now works with transcation groups. Just check
that there is no restriction in the transaction groups doc concerning
undo.

related to #14799

The undo/redo is implemented using SAVEPOINT.

The QgsTransaction interface has been enlarged to allow savepoints
creation and management. The savepoint is destroyed on
rollbackToSavepoint to have the same behavior has the sql ROLLBACK TO
SAVEPPOINT.

To avoid the creation of a savepoint for each feature modified in bulk
editing (e.g. paste, field calculator) the logic is a bit complicated: the
savepoint is created on QgsVectorLayer::editCommandStarted and the first
actual undo command (QgsVectorLayerUndoPassthroughCommand) is
responsible for the re-creation of the savepoint in case of undo-redo.
Since the behavior must be different in case edition doesn't take place
inside an edit command, a member function has been added to
QgsVectorLayer to expose the mEditCommandActive state.

Another (commented) tricky bit is the modification of the database
structure on add/delete attributes. On undo, the attribute is removed
before the rollback to savepoint, i.e. there is a useless ALTER TABLE
issued to restore the structure just before restoring it with the
ROLLBACK TO SAVEPOINT. This is necessary to make the provider
aware of the change of structure. It could be nicer/cleaner to have a way
to reload providers metadata.

The editPaste function has also been modified to use addFeatures instead of
addFeature (plural/singular), this is at the expense of an additional "cpy"
of the clipboard in memory, but it should improve perf with postgis provider.
@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Sep 8, 2017

Contributor

I don't understand the failure on ProcessingQgisAlgorithmsTest (segfault), doesn't crash on my machine.

I rebased/squashed fixups to cleanup and to trigger another build.

Contributor

vmora commented Sep 8, 2017

I don't understand the failure on ProcessingQgisAlgorithmsTest (segfault), doesn't crash on my machine.

I rebased/squashed fixups to cleanup and to trigger another build.

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg Sep 15, 2017

Contributor

@m-kuhn Any objection for a merge?

Contributor

haubourg commented Sep 15, 2017

@m-kuhn Any objection for a merge?

@m-kuhn

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Sep 15, 2017

Member

Would be great to quickly look through the code and find usages of operator aliases (which we nowhere else use in the codebase) and then it's ready to roll, looks like you have tested in-depth already.

Awesome stuff!

Member

m-kuhn commented Sep 15, 2017

Would be great to quickly look through the code and find usages of operator aliases (which we nowhere else use in the codebase) and then it's ready to roll, looks like you have tested in-depth already.

Awesome stuff!

@m-kuhn m-kuhn merged commit f63c302 into qgis:master Sep 15, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Sep 15, 2017

Contributor

@m-kuhn I was not even aware of operator aliases, thanks for your vigilance, now git diff origin/master |grep ' and ' says I'm clean.

Contributor

vmora commented Sep 15, 2017

@m-kuhn I was not even aware of operator aliases, thanks for your vigilance, now git diff origin/master |grep ' and ' says I'm clean.

@vmora

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Sep 15, 2017

Contributor

@pblottiere, @m-kuhn thanks a lot for your reviews

Contributor

vmora commented Sep 15, 2017

@pblottiere, @m-kuhn thanks a lot for your reviews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment