andreikop / enki

A text editor for programmers
http://enki-editor.org
GNU General Public License v2.0
161 stars 39 forks source link

Switching away from Markdown-preview-enabled pane and back disturbs viewport position. #319

Closed vi closed 8 years ago

vi commented 8 years ago
  1. The document is positioned to the very end. There is Markdown preview pane at the bottom: 1
  2. Let's switch to previous file: 2
  3. Let's switch back: 3

It failed to preserve viewport position. Fortunately when I try to type something, it does jump to the cursor.

bjones1 commented 8 years ago

I can't seem to reproduce this. What version of Enki are you using?

vi commented 8 years ago

b48e7b59df3048cb06aa7b6e07983406eb3a38cf

bjones1 commented 8 years ago

Hmmm. Is this a regression in the fuzzyopen branch? i.e. do you see in in master's head (https://github.com/hlamer/enki/commit/36975c884f7e39bd7b622c42c194e41fc36d1618)?

andreikop commented 8 years ago

@vi, could you check the master? I'd like to close this issue before releasing next Enki

vi commented 8 years ago

Not yet... Maybe get round and re-build from master now. I suppose I don't need to touch qutepart.

vi commented 8 years ago

I have tried using commit 921a546a5cffa4118ff2127e84a5fd9dd6745cca and got this error on start:

Traceback (most recent call last):
  File "/home/vi/src/git/enki/bin/enki", line 20, in <module>
    sys.exit(enki.main.main())
  File "/mnt/src/git/enki/bin/../enki/main.py", line 277, in main
    core.init(profiler, cmdLine)
  File "/mnt/src/git/enki/bin/../enki/core/core.py", line 135, in init
    self._loadPlugin(name)
  File "/mnt/src/git/enki/bin/../enki/core/core.py", line 198, in _loadPlugin
    exec("import enki.plugins.%s as module" % name)  # pylint: disable=W0122
  File "<string>", line 1, in <module>
  File "/mnt/src/git/enki/bin/../enki/plugins/fuzzyopen/__init__.py", line 2, in <module>
  File "/mnt/src/git/enki/bin/../enki/plugins/fuzzyopen/fuzzyopen.py", line 6, in <module>
ImportError: cannot import name StatusCompleter
vi commented 8 years ago

It was because of stale pyc files.

Checked with 1afeb4f7e059bccfed2d65202aa7659ebc8a9260 (v15.05.0) - the same bug with switching from/to review.

vi commented 8 years ago

v14.07.2 - the same. v12.06 - pane position gets reset to the right side on each switch => not applicable

Can't easily find intermediate version that does not fail to start.

bjones1 commented 8 years ago

OK, I finally reproduced this. Working on it.

Conditions: when the preview pane is above or below the text window, and a switch from a preview-enabled file to a non-previewable file then back is made, the text window is scrolled incorrectly. However, the caret retains its correct position. In general, the text pane should never be scrolled on a switch. The preview pane should be synchronized, though.

bjones1 commented 8 years ago

@hlamer, can you reproduce this on 14d389bdd2ae806d5d8b098790e146ad66132fb4? I did some hacking by removing most of the preview plugin code. In the code below, the bug disappears if I comment out core.mainWindow().addDockWidget(Qt.RightDockWidgetArea, self._dock) and core.mainWindow().removeDockWidget(self._dock). I'm confused -- why is the qutepart window being scrolled at all? Here's the modified enki.plugins.preview :

# **********************************************************
# __init__.py - The HTML, Markdown, and reST preview package
# **********************************************************
# The Preview plugin provides an HTML-based rendering of the
# file currently being edited. This file implements the
# Plugin interface; other modules are given below.
#
# Imports
# =======
# These are listed in the order prescribed by `PEP 8
# <http://www.python.org/dev/peps/pep-0008/#imports>`_.
#
#
# Third-party imports
# -------------------
from PyQt4.QtCore import QObject, Qt
from PyQt4.QtGui import QIcon
#
# Local imports
# -------------
from enki.core.core import core
from enki.widgets.dockwidget import DockWidget

# Plugin
# ======
# This class integrates the preview dock into Enki. Specifically, it:
#
# #. Adds the GUI defined above to the Settings dialog box
class Plugin(QObject):
    """Plugin interface implementation.
    """
    def __init__(self):
        """Create and install the plugin
        """
        QObject.__init__(self)
        self._dock = DockWidget(core.mainWindow(), "Previe&w", QIcon(':/enkiicons/internet.png'), "Alt+W")
        self._dockInstalled = False
        core.workspace().currentDocumentChanged.connect(self._onDocumentChanged) # Disconnected.

    def del_(self):
        """Uninstall the plugin
        """
        if self._dockInstalled:
            self._removeDock()
        core.workspace().currentDocumentChanged.disconnect(self._onDocumentChanged)

    def _onDocumentChanged(self):
        """Document or Language changed.
        Create dock, if necessary
        """
        if self._canPreview(core.workspace().currentDocument()):
            if not self._dockInstalled:
                self._createDock()
        else:
            if self._dockInstalled:
                self._removeDock()

    def _canPreview(self, document):
        """Check if the given document can be shown in the Preview dock.
        """
        if document is None:
            return False

        if document.qutepart.language() in ('Markdown', 'Restructured Text') :
            return True

        return False

    def _createDock(self):
        """Install dock
        """
        core.mainWindow().addDockWidget(Qt.RightDockWidgetArea, self._dock)
        self._dockInstalled = True
        self._dock.show()

    def _removeDock(self):
        """Remove dock from GUI
        """
        core.mainWindow().removeDockWidget(self._dock)
        self._dockInstalled = False
andreikop commented 8 years ago

Removing and adding the Preview dock changes the window geometry. This is not common behavior for Qt programs and it seems like Qt doesn't handle this case well.

I pushed to v15.11.0 81c39a66a8b084b471d5c939e610d6d187986ba6. After this commit cursor remains visible on the top or bottom line of the visible area, but the text is scrolled. It is a bad fix.

Do you have better ideas?

bjones1 commented 8 years ago

I'm close to a fix...hopefully tomorrow...

bjones1 commented 8 years ago

Some notes on my fix: the root cause, as noted by @hlamer, is that adding a dock causes Qt to lose the scroll bar position in the qutepart window.

Root cause

The sequence of events, based on screenshots from b64317fa4b9a8a75e7f8a1b2ae0e7c3acd4f7d27:

  1. Word wrap is on. Qt seems to save and restore window geometry based on the top line visible in the qutepart window, not counting wrapping. In addition, the cursor is positioned on a line which will be off screen after wrapping due to adding a dock in the next step.

    image

  2. A dock is added, which is wide enough to cause wrapping. Now, the cursor isn't visible. This is the root cause of the bug reported. Note that line 454, at the top of the screen earlier, is still at the top of the screen. However, the cursor is still at line 471, which is now offscreen.

    image

Why this is most obvious with the preview window:

  1. The preview dock is open and the cursor is on a line toward the bottom of the screen. The preview window causes qutepart to wrap several lines. For example, the cursor is on line 471.

    image

  2. The user switches to a different file with no preview. The preview dock is closed and qutepart loads and displays the other file's text.
  3. The user switches back to the preview-able file. So, first qutepart loads and displays the first file's text, with less wrapping since the preview dock hasn't been added yet. Here, line 454 is visible.

    image

  4. The preview dock is added. Line 454, the line visible at the top of the qutepart window before adding the dock, remains visible after adding the dock, which means the line at the cursor (line 471) is now scrolled off the screen.

    image

Fixes

The root cause is that the line at the top of the screen is being preserved, rather than the line at the cursor. So, the best fix would be to save the position of the cursor relative to the screen, then restore it. That is, if the cursor is 1/3 from the top of the screen before the dock, make sure it's 1/3 from the top after adding the dock.

For simplicity, ensuring that the cursor is at least visible seems a reasonable approximation of this fix. So, @hlamer's fix in 81c39a66a8b084b471d5c939e610d6d187986ba6 seems like a good fix, not a bad fix. While there's possibly a better fix, I think this is good enough.

@vi, what do you think? Would you test and see if this fixes the bug you originally reported?

bjones1 commented 8 years ago

Aargh. Qt strikes back. On Windows, running test_preview.py:

test_html (__main__.TestPreview) ... ok
test_logWindowSplitter1 (__main__.TestPreview) ... RuntimeError: wrapped C/C++ object of type Qutepa
rt has been deleted
RuntimeError: wrapped C/C++ object of type Qutepart has been deleted
ok
test_logWindowSplitter2 (__main__.TestPreview) ... RuntimeError: wrapped C/C++ object of type Qutepa
rt has been deleted
RuntimeError: wrapped C/C++ object of type Qutepart has been deleted
ok
test_logWindowSplitter3 (__main__.TestPreview) ... RuntimeError: wrapped C/C++ object of type Qutepa
rt has been deleted
ok
test_logWindowSplitter3a (__main__.TestPreview) ... RuntimeError: wrapped C/C++ object of type Qutep
art has been deleted
ok
test_logWindowSplitter4 (__main__.TestPreview) ... RuntimeError: wrapped C/C++ object of type Qutepa
rt has been deleted
ok
test_markdown (__main__.TestPreview) ... RuntimeError: wrapped C/C++ object of type Qutepart has bee
n deleted
ok
test_markdown_templates (__main__.TestPreview) ... RuntimeError: wrapped C/C++ object of type Qutepa
rt has been deleted
ok
test_markdown_templates_help (__main__.TestPreview) ... RuntimeError: wrapped C/C++ object of type Q
utepart has been deleted
ok

...and so on. I assume that what's happening is the timer only expires after the test is over and the qutepart widget has been destroyed.

andreikop commented 8 years ago

I see. The fix is worse than the bug. v15.11 will be released without this fix

vi commented 8 years ago

The priority is mid. Locator improvements, split view and so on may be more important.

Can there be a hack that if preview is scrolled to the very bottom, then after switching it also gets scrolled to the very bottom? Scenario is appending to README.md periodically looking at source code.

bjones1 commented 8 years ago

I have an alternate fix that we could try if you'd like. I'll see if I can find it and push it soon.

bjones1 commented 8 years ago

Per @hlamer, #350 doesn't fix the problem either. The root cause is that the state of the qutepart window should be saved before the dock is closed, and restored after the dock is opened. I'm not quite sure how to accomplish this, though.

bjones1 commented 8 years ago

Another thought on this: I think @hlamer's fix in https://github.com/hlamer/enki/commit/81c39a66a8b084b471d5c939e610d6d187986ba6 is fine, but the unit tests don't handle the fix well. So, perhaps a fix to the unit testing framework is better.

andreikop commented 8 years ago

I tried to apply my fix on Py3 and Qt5 and there are no unit test crashes. @bjones1, could you check master now?

bjones1 commented 8 years ago

No unit test crashes and this does fix the bug. Playing, I noticed a variants of this bug that still exists:

  1. Putting the cursor at the bottom of the screen: image
  2. Then showing a dock causes the cursor to be pushed off the bottom of the screen, making it "disappear". image

Again, this is the same underlying bug.