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

PR: Fix EOL status bug. #2393

Closed
wants to merge 2 commits into from
Closed

PR: Fix EOL status bug. #2393

wants to merge 2 commits into from

Conversation

ssoloff
Copy link
Contributor

@ssoloff ssoloff commented May 2, 2015

There were three root causes for the symptoms described in the original issue.

  1. Incorrect EOL menu item state

Editor.toggle_eol_chars was being called when the associated widget was both checked and unchecked (apparently, Qt, like most windowing systems, automatically raises a toggled event for the previously selected radio button when a new radio button is toggled). However, toggle_eol_chars appears to assume it is only invoked in the case when the toggled event's checked attribute is True.

When the user selects a new EOL menu item, toggle_eol_chars is invoked twice: once for the radio button being unchecked and once for the radio button being checked. Thus, BaseEditMixin.set_eol_chars is also invoked twice: once with the new EOL and once with the old EOL. set_eol_chars eventually causes the refresh_eol_chars signal to be emitted (via EditorStack.modification_changed). Thus, Editor.refresh_eol_chars is invoked twice with different EOLs. As with toggle_eol_chars, refresh_eol_chars assumes it is only invoked for the new EOL, as evidenced by the fact that it only ever calls setChecked(True) on the associated EOL menu item.

The fix is to ensure toggle_eol_chars only handles the checked case.

  1. Stale status bar EOL value

BaseEditMixin.set_eol_chars invokes self.document().setModified(True) before setting self.eol_chars to the new EOL. Setting the document's modified flag causes EditorStack.modification_changed to be invoked synchronously. modification_changed emits the refresh_eol_chars signal based on the current value of self.eol_chars. Because self.eol_chars at this point still references the old EOL, the status bar signal handler uses a stale value to update the UI.

The fix is to ensure self.eol_chars is set to the new EOL before invoking self.document().setModified(True).

  1. No EOL menu item updates while document state is modified

BaseEditMixin.set_eol_chars invokes self.document().setModified(True), which subsequently causes a refresh_eol_chars signal to be emitted, which subsequently causes Editor.refresh_eol_chars to be invoked to update the EOL menu item checked state. If the document state is already marked as modified, invoking setModified(True) again does not cause the editor's modificationChanged event to be raised again. Thus, Editor.refresh_eol_chars is not called and the EOL menu items are not updated again until the document state changes from modified to unmodified (e.g. by saving the document).

The fix is to force the document to think its being moved from the unmodified state to the modified state by first calling setModified(False) before calling setModified(True). This smells really bad. Unfortunately, I don't know enough about the Qt API to know if there's a better way to force the refresh_eol_chars signal to be raised in this case. My naive understanding of this part of the code would suggest that it might be better to extract a new method on EditorStack responsible for setting eol_chars. That new method could emit the refresh_eol_chars signal as part of changing the EOL. It seems the emission of that signal should be independent of the document modification state changing. I'm hoping one of the committers can improve this portion of the fix.

@ssoloff
Copy link
Contributor Author

ssoloff commented May 2, 2015

For #2392.

@ccordoba12 ccordoba12 added this to the v3.0 milestone May 6, 2015
@ccordoba12
Copy link
Member

About your point 3): please follow your own suggestion to:

extract a new method on EditorStack responsible for setting eol_chars

@ccordoba12
Copy link
Member

I think it's the cleanest way to implement this :-)

@ssoloff
Copy link
Contributor Author

ssoloff commented May 7, 2015

Thanks for the review, Carlos. I should have time to make the suggested changes before the end of the weekend.

ssoloff added 2 commits May 8, 2015 20:10
There were three root causes for the symptoms described in the original issue.

1) Incorrect EOL menu item state

Editor.toggle_eol_chars was being called when the associated widget was both checked and unchecked (apparently, Qt, like most windowing systems, automatically raises a toggled event for the previously selected radio button when a new radio button is toggled).  However, toggle_eol_chars appears to assume it is only invoked in the case when the toggled event's checked attribute is True.

When the user selects a new EOL menu item, toggle_eol_chars is invoked twice: once for the radio button being unchecked and once for the radio button being checked.  Thus, BaseEditMixin.set_eol_chars is also invoked twice: once with the new EOL and once with the old EOL.  set_eol_chars eventually causes the refresh_eol_chars signal to be emitted (via EditorStack.modification_changed).  Thus, Editor.refresh_eol_chars is invoked twice with different EOLs.  As with toggle_eol_chars, refresh_eol_chars assumes it is only invoked for the new EOL, as evidenced by the fact that it only ever calls setChecked(True) on the associated EOL menu item.

The fix is to ensure toggle_eol_chars only handles the checked case.

2) Stale status bar EOL value

BaseEditMixin.set_eol_chars invokes self.document().setModified(True) *before* setting self.eol_chars to the new EOL.  Setting the document's modified flag causes EditorStack.modification_changed to be invoked synchronously.  modification_changed emits the refresh_eol_chars signal based on the current value of self.eol_chars.  Because self.eol_chars at this point still references the old EOL, the status bar signal handler uses a stale value to update the UI.

The fix is to ensure self.eol_chars is set to the new EOL before invoking self.document().setModified(True).

3) No EOL menu item updates while document state is modified

BaseEditMixin.set_eol_chars invokes self.document().setModified(True), which subsequently causes a refresh_eol_chars signal to be emitted, which subsequently causes Editor.refresh_eol_chars to be invoked to update the EOL menu item checked state.  If the document state is already marked as modified, invoking setModified(True) again does not cause the editor's modificationChanged event to be raised again.  Thus, Editor.refresh_eol_chars is not called and the EOL menu items are not updated again until the document state changes from modified to unmodified (e.g. by saving the document).

The fix is to force the document to think its being moved from the unmodified state to the modified state by first calling setModified(False) before calling setModified(True).  *This smells really bad.*  Unfortunately, I don't know enough about the Qt API to know if there's a better way to force the refresh_eol_chars signal to be raised in this case.  My naive understanding of this part of the code would suggest that it might be better to extract a new method on EditorStack responsible for setting eol_chars.  That new method could emit the refresh_eol_chars signal as part of changing the EOL.  It seems the emission of that signal should be independent of the document modification state changing.  I'm hoping one of the committers can improve this portion of the fix.
Previously, we relied on an editor modification state changed signal to inform the editor stack of an EOL change.  Because this signal is only emitted when the editor modification state changes, we were forced to reset the state to False before setting it to True in the case when it was already True.  This smelled bad.

This change adds a new signal to TextEditBaseWidget for the sole purpose of informing observers that an EOL change has occurred.  The editor stack now subscribes to this signal, which causes the editor stack's refresh_eol_chars signal to be emitted after an EOL change even if no editor modification changed signal arrives.  Adding a new signal seemed cleaner than making BaseEditMixin aware of the editor stack in order to invoke a method explicitly to set the new EOL.

Note that in the case where the document is initially clean, two refresh_eol_chars signals will be emitted by the editor stack after an explicit EOL change: one when the editor modification changed signal is received and one when the EOL changed signal is received.  While undesirable, this should be harmless.
@ssoloff
Copy link
Contributor Author

ssoloff commented May 9, 2015

I didn't quite follow the plan I documented in my original commit. After further review, I noted no precedent for BaseEditMixin (or any other mixin) being aware of the editor stack. So I instead decided to add a new signal used to notify interested observers (the editor stack in this case) that the editor's EOL has changed.

Some things I'd appreciate feedback on:

  • Not sure if the new method EditorStack.refresh_eol has the best name. I was originally going to name it refresh_eol_chars to be consistent with the other related members throughout the code, but it obviously conflicts with the EditorStack class field of the same name.
  • Should EditorStack.refresh_eol accept the EOL chars or the OS name? Using EOL chars is simpler and keeps the responsibility for converting EOL chars to OS name in one place, but it may not be the most logical choice from the domain API perspective.
  • Please advise if it is sufficient to add the eol_chars_changed signal field to just TextEditBaseWidget or if the same field must be added to other types that inherit BaseEditMixin (e.g. IPythonControlWidget and IPythonPageControlWidget). I don't believe these types will ever have the BaseEditMixin.set_eol_chars method called.

@goanpeca goanpeca changed the title Fix EOL status bug. PR: Fix EOL status bug. Jan 27, 2016
@ccordoba12 ccordoba12 modified the milestones: v3.0, v3.0beta4 Jan 27, 2016
@ccordoba12 ccordoba12 modified the milestones: v3.1, v3.0beta4 Jun 9, 2016
@ccordoba12
Copy link
Member

@rlaverde, please rebase this PR, create a new one with the result and finish it.

@ccordoba12 ccordoba12 removed this from the v3.1 milestone Nov 23, 2016
@ccordoba12
Copy link
Member

Closing in favor #3733.

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

Successfully merging this pull request may close these issues.

3 participants