PR: Fix EOL status after changing it #3733
Conversation
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.
|
My answers to @ssoloff concerns:
Please rename that method to
I think it's fine to make this new method to accept EOL chars instead of the OS name. So please leave it as @ssoloff defined it.
Yep, it's enough to add that signal to |
signal_refresh_eol_chars
|
@rlaverde, you need to merge your PR with 3.x to pick up the chardet dependency. |
| @@ -280,7 +280,7 @@ class EditorStack(QWidget): | |||
| readonly_changed = Signal(bool) | |||
| encoding_changed = Signal(str) | |||
| sig_editor_cursor_position_changed = Signal(int, int) | |||
| refresh_eol_chars = Signal(str) | |||
| signal_refresh_eol_chars = Signal(str) | |||
There was a problem hiding this comment.
We follow the convention of naming signals with the sig_ prefix, not signal_
| @@ -222,6 +222,7 @@ class TextEditBaseWidget(QPlainTextEdit, BaseEditMixin): | |||
| zoom_out = Signal() | |||
| zoom_reset = Signal() | |||
| focus_changed = Signal() | |||
| eol_chars_changed = Signal(str) | |||
There was a problem hiding this comment.
Please also rename this new signal as sig_eol_chars_changed (and let's try to do that for all new signals we introduce :-).
Fix error, wrong signal connection in editorstack
|
LGTM, thanks @rlaverde! |
ccordoba12
changed the title
Fixes #2392
Rebase of #2393 created by @ssoloff
how I should address @ssoloff comments in #2393 (comment) ?