andreikop / enki

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

Bottom of editor window doesn't show last line in file #191

Closed bjones1 closed 10 years ago

bjones1 commented 10 years ago

If I scroll to the last line of a file (I saw this with preview.py from the latest commit), close Enki, then re-run it, the last line will be scrolled off the bottom of the screen. Down arrow movement / scrollbar mouse drags can never reach this hidden line. The only way to get the lost line visible is to resize the Enki window.

I think what's happening is that the horizontal scrollbar is on briefly but then hidden, but the screen space it occupied (which covers the bottom line) doesn't get taken in to account when calculating screen dimensions. That's just a guess...

andreikop commented 10 years ago

Hmm, very strange. I can't reproduce this bug. Maybe you could try to write a simple test? (Create Qutepart, resize, set text, set cursor position, show)

bjones1 commented 10 years ago

Sounds good. I'll work on it.

bjones1 commented 10 years ago

I discovered that I can only reproduce the bug when I quit then restart Enki, I can't find a way to reproduce this in a unit test, though. My attempt so far:

    def test_7(self):
        self._test_7a()
        self._test_7b()

    @base.inMainLoop
    def _test_7a(self):
        """Test that the last line in the qutepart window is visible."""
        # To reproduce this bug:
        #
        # Create a file with lots of lines (more than will fit on the screen).
        core.config()["Qutepart"]["Wrap"]["Enabled"] = False
        path = 'file1.txt'
        self.createFile(path, 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\r\n'*1000 + 'a')
        # Move to the last line.
        self.keyClick('Ctrl+Enter')
        self.keyClick('Ctrl+End')
        QTest.qWait(1000)
        self.app.quit()

    @base.inMainLoop
    def _test_7b(self):
        # Re-open it.
        core.workspace().openFile('file1.txt')
        # Check the cursor location.
        QTest.qWait(4000)

An image from the clipboard showing the problem. It only occurs when the vertical height is "enough", about 3/4 of my screen.

image

andreikop commented 10 years ago

I can not reproduce the bug. Have you seen in on Linux?

If it reproduces stable for you, could you try to do next changes in Qutepart:

bjones1 commented 10 years ago

I can't reproduce in Linux (Ubuntu 12.04 LTS) either.

I made the changes above to qutepart, installed, then tested in Enki. I still see the problem after making the changes given above.

I noticed that on Unix, the bottom of the qutepart window doesn't com as close to the bottom of Enki as it does on Windows. Perhaps this spacing difference is what's causing the problem.

andreikop commented 10 years ago

Hmm, I can't reproduce this bug on windows either before #194 was fixed and on current master. But, Bryan, could you please check current master?

bjones1 commented 10 years ago

I still see the same problem. As Enki starts, I notice that the vertical size of the text in the qutepart window increases after load; I think what I'm seeing is the syntax highligher run, which changes the line height. Is there an easy way to disable the syntax highlighter to see if the problem goes away?

andreikop commented 10 years ago

Immediately return from Qutepart.detectSyntax()

On Wed, Apr 2, 2014 at 7:07 PM, bjones1 notifications@github.com wrote:

I still see the same problem. As Enki starts, I notice that the vertical size of the text in the qutepart window increases after load; I think what I'm seeing is the syntax highligher run, which changes the line height. Is there an easy way to disable the syntax highlighter to see if the problem goes away?

— Reply to this email directly or view it on GitHubhttps://github.com/hlamer/enki/issues/191#issuecomment-39349302 .

bjones1 commented 10 years ago

That fixes it!

How else can I help track this one down?

andreikop commented 10 years ago

Hmm. It seems like it is a Qt bug. Control height changes after highlighting is applied. I don't know how to workaround it. Will ask Qt guru.

bjones1 commented 10 years ago

Sounds good. I don't see the control height changing, but the height of each line does change after highlighting.

On a side note, that's something Ihat's broken or inaccurate in my code -- I know a QPlainTextEdit allows lines with different fonts / heights, but I don't check for that case and assume that everything is the same height. Do you know of a nice Qt way to do a hit test on a QPlainTextEdit window -- that is, what line does a given (x, y) coordinate correspond to?

andreikop commented 10 years ago

I think monospace plain text Must be mono-height and it is a Qt bug. Don't care about different height in your code 03.04.2014 20:32 пользователь "bjones1" notifications@github.com написал:

Sounds good. I don't see the control height changing, but the height of each line does change after highlighting.

On a side note, that's something Ihat's broken or inaccurate in my code -- I know a QPlainTextEdit allows lines with different fonts / heights, but I don't check for that case and assume that everything is the same height. Do you know of a nice Qt way to do a hit test on a QPlainTextEdit window -- that is, what line does a given (x, y) coordinate correspond to?

— Reply to this email directly or view it on GitHubhttps://github.com/hlamer/enki/issues/191#issuecomment-39480380 .

andreikop commented 10 years ago

PasNox: call textedit->updateGeometry()

andreikop commented 10 years ago

Bryan, could you test branch

andreikop commented 10 years ago

Bryan, could you please test Qutepart branch 191_fix?

I don't want to include the fix to mainline unless it really necessary, so check please both master and 191_fix to make sure my commit really fixes the problem.

bjones1 commented 10 years ago

Andrei,

Thanks for working on this.

With master (0f06bfd1c59f503124e32f8c7750182b43980b0e), I get this:

image

With 191_fix:

image

So, the fix doesn't seem to help...

bjones1 commented 10 years ago

However, on either version, pressing PgUp followed by PgDn fixes it (the last line is made visible). Perhaps there's some sort of workaround possible?

pasnox commented 10 years ago

Andrei, if the textEdit->updateGeometry() does not help, you could also try textEdit->viewport()->updateGeometry(). You could also have a look here: https://qt.gitorious.org/qt/qtbase/source/3245745cd0d843a8927dbc2015b8f3f416a41cdb:src/widgets/widgets/qplaintextedit.cpp#L773 It looks like they have something private to update the scrollbar: _q_adjustScrollbars() The easiest way to trigger it is to send QResizeEvent to the widget instead of calling updateGeometry, something like that:

QResizeEvent event( size(), size() ); QApplication::sendEvent( this, &event );

bjones1 commented 10 years ago

Based on paxnox's suggestion above and the link s/he provided, it looks like the signal documentSizeChanged() causes _q_adjustScrollbars() to be called. So, I modified qutepart's syntaxhighlighter to emit the signal, and it works! ...mostly. I can see the flicker of the update, but the last line ends up scrolled off the screen. Pressing the down arrow then scrolls to the bottom; without this change, pressing the down arrow has no effect. Is there some way to call ensureCursorVisible() after the change below?

My code, placed at the very bottom of qsyntaxhighlighter.py:

        dl = self._textEdit.document().documentLayout()
        dl.documentSizeChanged.emit(dl.documentSize())
pasnox commented 10 years ago

To avoid the flickering you should emit it only once a time. You should base on the code hlamer done using the QTimer in single shot mode. This way the size changed will be emitted only once (or very few times, compared to each document change times).

bjones1 commented 10 years ago

pasnox,

Thanks for the idea. The flicker happens just once, on startup. As long as this code gets called after the syntax highlight occurs and before __init__ finishes, I think we're fine as it. Unfortunately, I don't know the qutepart code well enough to know where these two lines belong, but I believe Andrei can take this plus a call to self.ensureCursorVisible to close this bug.

andreikop commented 10 years ago

I moved code by Bryan to call it when highlighting is finished. @bjones1 , could you please test qutepart master and close the bug if it is ok now?

bjones1 commented 10 years ago

I'm back from vacation...and I can't exactly reproduce the bug. This is what I get now: the line numbers don't match up with the line, rather than scrolling a line off the bottom of the screen. I haven't changed anything significant in my PC (other than leaving for a couple of weeks).

image

Trying the branch produces an exception:

CRITICAL:root:Traceback (most recent call last):
  File "C:\Users\bjones\Documents\enki_all\enki\bin\..\enki\plugins\session.py",
 line 44, in _onRestoreSession
    core.workspace().openFile(filePath)
  File "C:\Users\bjones\Documents\enki_all\enki\bin\..\enki\core\workspace.py",
line 561, in openFile
    document = self._openSingleFile(filePath)
  File "C:\Users\bjones\Documents\enki_all\enki\bin\..\enki\core\workspace.py",
line 542, in _openSingleFile
    document = Document(self, filePath)
  File "C:\Users\bjones\Documents\enki_all\enki\bin\..\enki\core\document.py", l
ine 204, in __init__
    self._tryDetectSyntax()
  File "C:\Users\bjones\Documents\enki_all\enki\bin\..\enki\core\document.py", l
ine 216, in _tryDetectSyntax
    firstLine=self.qutepart.lines[0])
  File "C:\Python27\lib\site-packages\qutepart\__init__.py", line 597, in detect
Syntax
    self._highlighter = SyntaxHighlighter(syntax, self.document())
  File "C:\Python27\lib\site-packages\qutepart\syntaxhlighter.py", line 108, in
__init__
    self._onContentsChange(0, 0, charsAdded, zeroTimeout=self._wasChangedJustBef
ore())
  File "C:\Python27\lib\site-packages\qutepart\syntaxhlighter.py", line 220, in
_onContentsChange
    self._highlighBlocks(firstBlock, untilBlock, timeout)
  File "C:\Python27\lib\site-packages\qutepart\syntaxhlighter.py", line 282, in
_highlighBlocks
    documentLayout = self._textEdit.document().documentLayout()
AttributeError: 'SyntaxHighlighter' object has no attribute '_textEdit'

Rebasing master on to 191_fix fixes this problem. However, the (new?) bug (line numbers don't line up) doesn't go away. I'll play with this more tomorrow to see if I can reproduce the old bug.

In the meantime, I'd suggest adding a link to this issue to explain what the two added lines actually do.

bjones1 commented 10 years ago

An update: I finally reproduced the bug: it's only occurring when I set the font to a Courier New at 9 points. Neither 8 nor 10 points shows it.

Using the updated code helps: now, I can at least scroll down to see the line, but the cursor starts off the bottom of the screen. Adding a call to self._textEdit.ensureCursorVisible() fixes this for me, but this may break other areas of the code (disallow scrolling the cursor off the screen if a highlight is triggered?). A screenshot on startup, without this call:

image

andreikop commented 10 years ago

Hmm, I don't understand how using the updated code helps if the code crashes.

I can't remember how did I produce this broken commit, however I tried to fix it. Check the latest qutepart master

bjones1 commented 10 years ago

Better. Adding self._textEdit.ensureCursorVisible() after line 280 now fixes this, but may have other negative effects as mentioned above.

A thought: given that there seems at least one other bug related to this issue (line numbers incorrect at startup), another approach that could solve both would be to delay calling _highlightBlocks() until after Enki has fully started up, e.g. with a QTimer.singleShot(0, _syntaxHighlighter, _highlightBlocks) rather than calling _onContentsChange (which calls _highlightBlocks) in __init__. Would you mind creating a branch with this fix?

andreikop commented 10 years ago

Could you please test branch 191_fix_v3?

andreikop commented 10 years ago

If 191_fix_v3 doesn't work, how about just closing this issue? Too much time for the minor bug

pasnox commented 10 years ago

I could try to take over the issue to fix it. Just email me what to do to get the code and reproduce the bug on linux.

bjones1 commented 10 years ago

@pasnox: Thanks for the offer. Unfortunately, I can't reproduce this on Linux, only on Windows. Sigh...

bjones1 commented 10 years ago

@hlamer: 191_fix_v3 doesn't work -- sorry, I thought that would help. The current changes in master fix 99% of the bug, so I'm happy and will close.