Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#13739 closed defect (fixed)

Improved Undo/Redo wxCommandProcessor::IsDirty()

Reported by: nikospic Owned by:
Priority: low Milestone:
Component: GUI-generic Version: 2.8.12
Keywords: wxCommandProcessor::IsDirty() Cc:
Blocked By: Blocking:
Patch: no

Description

In file include/wx/cmdproc.h

I am under the impression that this is the correct implementation for IsDirty()

virtual bool IsDirty() const
{

return m_commands.empty()
( m_lastSavedCommand != m_currentCommand );

}

The problem is that the original implantation essentially checks if you are at the beginning of the commands list:

return m_currentCommand && (m_lastSavedCommand != m_currentCommand);

however we simply need to know if there more commands in the list.

Example where the original does not work:

let's say we have an empty text:
""

Then we edit:
(0) ""
(1) "A"
(2) "AB"
(3) "ABC"

We then undo once, which returns us to state (2) and save.

So undoing and redoing always shows that (2) was the saved stage but when we go back to (0) "" the we are told that we do not need to save!

Change History (9)

comment:1 Changed 9 years ago by vadz

Your code for IsDirty() probably got mangled, could you please repost it inside triple curly braces to show it verbatim?

comment:2 Changed 9 years ago by nikospic

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

The new version should be:

virtual bool IsDirty() const
{
   return m_commands.empty() ( m_lastSavedCommand != m_currentCommand );
}

instead of:

return m_currentCommand && (m_lastSavedCommand != m_currentCommand);

comment:3 Changed 9 years ago by vadz

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry, your code still appears mangled. Is there a missing || there by chance?

You also probably didn't mean to close it...

comment:4 Changed 9 years ago by nikospic

Yes indeed the OR operator (||) is missing:

bool dCommandProcessor::IsDirty() const
{
  return m_commands.empty() || ( m_lastSavedCommand != m_currentCommand );
}

comment:5 follow-up: Changed 9 years ago by vadz

Now it's syntactically valid but doesn't make sense to me: why should IsDirty() return true when there are no commands at all (i.e. LHS of operator|| is true)?

I still didn't have time to test your example so I may be missing something but the current logic looks correct to me: we test that if we have any commands and, if we do have them, we're dirty if not all of them have been saved. Why is this wrong?

comment:6 in reply to: ↑ 5 Changed 9 years ago by nikospic

Replying to vadz:

Now it's syntactically valid but doesn't make sense to me: why should IsDirty() return true when there are no commands at all (i.e. LHS of operator|| is true)?

I still didn't have time to test your example so I may be missing something but the current logic looks correct to me: we test that if we have any commands and, if we do have them, we're dirty if not all of them have been saved. Why is this wrong?

This is because the correct way to "test that if we have any commands" is m_commands.empty() and not what we currently do (essentially whether m_currentCommand != NULL).

The latter is wrong because after some editing m_currentCommand becomes non NULL but there are command in the m_commands wxList.

comment:7 Changed 9 years ago by vadz

Ok but then it should be

!m_commands.empty() && (m_lastSavedCommand != m_currentCommand);

instead of your version from comment:4, shouldn't it? As I wrote in comment:5 it can't be right for IsDirty() to return true if there are no commands at all.

comment:8 Changed 9 years ago by vadz

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

I think this should be fixed in r70460, please reopen with the instructions about how the bug can be reproduced if you still see it.

Thanks!

comment:9 Changed 9 years ago by nikospic

My Apologies Vadim, I left for holidays and did not respond.
It seems to me that your fix is correct.
Thanks!

Note: See TracTickets for help on using tickets.