Opened 11 months ago

Closed 7 months ago

#15226 closed defect (fixed)

wxRichTextCtrl: Implement setting properties with undo for objects e.g. wxRichTextTable

Reported by: dghart Owned by:
Priority: normal Milestone: 3.0.0
Component: wxRichText Version: stable-latest
Keywords: Properties objects Cc:
Blocked By: Blocking:
Patch: yes

Description

It's currently possible to set properties with undo using wxRichTextParagraphLayoutBox::SetProperties. However this sets properties on a paragraph and its contents, which is inappropriate for a wxRichTextTable.

The attached patch adds a function that does this for an object. It can be 'seen' working in the unit-test patch.

Attachments (6)

richtextctrltest.diff download (1.9 KB) - added by dghart 11 months ago.
patch.diff download (3.2 KB) - added by dghart 7 months ago.
IgnoreFirstTime.patch download (2.5 KB) - added by dghart 7 months ago.
patch2.diff download (1.9 KB) - added by dghart 7 months ago.
patch3.diff download (4.4 KB) - added by dghart 7 months ago.
patch4.diff download (1.4 KB) - added by dghart 7 months ago.

Download all attachments as: .zip

Change History (16)

Changed 11 months ago by dghart

comment:1 Changed 11 months ago by vadz

Julian, is this OK to apply? I don't have any objections but then I don't understand anything in this code neither...

comment:2 Changed 10 months ago by vadz

  • Milestone changed from 2.9.5 to 3.0

This is not 2.9.5 critical, even if it would be great to apply it as soon as possible to get more testing for these patches. But I leave the decision as to whether do it right now (before 2.9.5) or later to Julian.

comment:3 Changed 10 months ago by vadz

This is not 2.9.5 critical, even if it would be great to apply it as soon as possible to get more testing for these patches. But I leave the decision as to whether do it right now (before 2.9.5) or later to Julian.

comment:4 Changed 10 months ago by vadz

This is not 2.9.5 critical, even if it would be great to apply it as soon as possible to get more testing for these patches. But I leave the decision as to whether do it right now (before 2.9.5) or later to Julian.

Changed 7 months ago by dghart

comment:5 follow-up: Changed 7 months ago by juliansmart

  • Status changed from new to confirmed

Hi David, I've applied this with some changes which I think make the code easier to read: using wxRichTextAction::SetOldAndNewObjects, we can set both the address of the original object, and the new object to be swapped in. I've been using this successfully in my own code; perhaps you can test it, and if it works for you, I'll delete the commented-out code and close this ticket.

I did consider adding a wxRICHTEXT_CHANGE_OBJECT_PROPERTIES command but I think adding the extra complexity probably isn't worth the efficiency saving in not having to save the entire object.

Thanks!

comment:6 in reply to: ↑ 5 Changed 7 months ago by dghart

Replying to juliansmart:

I've applied this with some changes which I think make the code easier to read: using wxRichTextAction::SetOldAndNewObjects, we can set both the address of the original object, and the new object to be swapped in. I've been using this successfully in my own code; perhaps you can test it, and if it works for you, I'll delete the commented-out code and close this ticket.

Your code is certainly neater. However testing here found some problems.

First the trivial one: SetProperties() isn't called in the 'else' section. Harder was a consequence of setting both objects at once, rather than deferring the StoreObject() call. This made SubmitAction() reverse the objects, so that initially nothing changed; Undo() then applied the change, Redo() removed it.

Maybe hackishly reversing the parameters of SetOldAndNewObjects() would have worked, but a better solution is to set wxRichTextAction's ignoreFirstTime parameter. I then found that this feature isn't currently implemented; the attached IgnoreFirstTime.patch does this. After applying this, the new method worked both in my code and in the unit-test patch.

Except that it only works for normal objects like a table: it asserts for a wxRichTextCell, as its parent isn't a wxRichTextParagraph (my local version had inadvertently worked-around this issue). Passing the table instead of the cell won't work, of course: the properties will be applied to that. I can't think of a better solution than to add an extra, normally null, parameter to take the objToSet the properties to (please improve the name!). After this, it really does work.

So there are 3 new patches: IgnoreFirstTime.patch as mentioned; patch2.diff which will fail for a cell; and patch3.diff which won't (patch2 and patch3 are alternatives). There's also richtextctrltest.diff. Please let me know if you intend to apply this; if so I'll improve it to test setting cell properties too.

Changed 7 months ago by dghart

Changed 7 months ago by dghart

Changed 7 months ago by dghart

comment:7 follow-up: Changed 7 months ago by juliansmart

Hi David,

Sorry about missing out the SetProperties call in the 'else' branch. Also I'm happy to add that extra object for setting cell properties.

However I'm a bit confused about why defer applying the new object on Submit. I know that some of your code (e.g. AddRows) modifies the *original* object and stores the clone, but in our new property setting function, we change and submit the clone. Therefore the clone should indeed replace the old object immediately, should it not? However, if objToSet is passed, then we would need to find the address of the old object, and then extract the new sub-object in the clone and set the properties on that. So that does add a bit more complexity to balance against the delay-swapping approach. On reflection, your approach probably has the edge :-) so I'll apply it shortly.

comment:8 in reply to: ↑ 7 Changed 7 months ago by dghart

However I'm a bit confused about why defer applying the new object on Submit. I know that some of your code (e.g. AddRows) modifies the *original* object and stores the clone, but in our new property setting function, we change and submit the clone. Therefore the clone should indeed replace the old object immediately, should it not?

If the clone is modified, that would presumably be right. But my first patch modified the original object, as does the 'if' branch of the current code.

I don't mind which way it's done. Are there any Properties-setting dialogs at the moment? If so, fitting in with their way might be best.

However, if objToSet is passed, then we would need to find the address of the old object, and then extract the new sub-object in the clone and set the properties on that. So that does add a bit more complexity to balance against the delay-swapping approach. On reflection, your approach probably has the edge :-) so I'll apply it shortly.

Thanks, but talking about the 'if' branch just now made me notice that patch3 doesn't consider that, so the wrong object may be changed when suppressing undo. Please wait while I fix it.


Changed 7 months ago by dghart

comment:9 Changed 7 months ago by dghart

Too late :) so I've created an additional patch with the fix.

comment:10 Changed 7 months ago by juliansmart

  • Resolution set to fixed
  • Status changed from confirmed to closed

OK, thanks! Applied. Nice and neat now.

Note: See TracTickets for help on using tickets.