DocumentPrivate: Add option "Enable Auto Reload" to ModOnHdPrompt
ClosedPublic

Authored by loh.tar on Mar 4 2019, 3:26 PM.

Details

Summary

This patch add an auto reload functionality when the user answers the ModOnHdPrompt accordingly

BUG:375361
BUG:377505
BUG:384384
FIXED-IN: 5.57

Test Plan

375361 Request a behaviour like tail -f, to fulfil this would an option needed to scroll to the bottom of the view or move the cursor there.
However, some comments are against this at all, but I think it's no harm to have it.
384384 Say "no tail -f" but it sound so

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
loh.tar created this revision.Mar 4 2019, 3:26 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptMar 4 2019, 3:26 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
loh.tar requested review of this revision.Mar 4 2019, 3:26 PM
loh.tar edited the summary of this revision. (Show Details)Mar 4 2019, 3:29 PM
loh.tar edited the test plan for this revision. (Show Details)
loh.tar edited the summary of this revision. (Show Details)Mar 4 2019, 4:19 PM
loh.tar edited the test plan for this revision. (Show Details)

I would like to expand this patch by "auto reload when file is empty"
Sometimes I have the case that a session is opened where some files are gone due to work on a different branch. Switching the branch then caused to ask unneeded.

cullmann requested changes to this revision.Mar 4 2019, 6:38 PM
cullmann added a subscriber: cullmann.

I think in principle the idea is sound.

But e.g., if you have have some file where one process writes to "fast" and it is "large" this will instantly lock up the application.

If we really want that, we need to: 1) ask the user e.g. if he wants to "auto-reload" this document on first change 2) have some timer based throtteling of the reloading to avoid lockups.

This revision now requires changes to proceed.Mar 4 2019, 6:38 PM
loh.tar updated this revision to Diff 53296.EditedMar 6 2019, 4:36 PM
loh.tar retitled this revision from DocumentPrivate: Auto reload in read-only mode to DocumentPrivate: Add option "Enable Auto Reload" to ModOnHdPrompt.
loh.tar edited the summary of this revision. (Show Details)
loh.tar edited the test plan for this revision. (Show Details)
  • Add option "Enable Auto Reload" to ModOnHdPrompt
  • Allow also auto reload for not read-only files
  • Only auto reload when doc is unmodified
  • Only reload when no reload is in progress, may that not enough to avoid mentioned lock up?


Issues:

  • The new button is slightly dangerous placed for my taste but with intend not nearby the normal reload button
  • No other way to disable that setting than to close/open the file
  • The reload is only triggered when the application has the focus, but that may be an intended behavior

TODO(?)

  • Add getter function so that e.g. the status bar can show a suitable icon to indicate that mode and the ModOnHdPrompt can hide that button in case of an already active setting
  • Add some option/action to the view so that the view scrolls down after an auto reload
cullmann requested changes to this revision.Mar 9 2019, 3:46 PM

Perhaps in addition to the message, one should just add a menu entry in Tools like "Read Only Mode", then the user can turn it of again.
And as said, I think one needs some timer to delay this to reload all x seconds, otherwise it might really stall.
Beside that: already nice

This revision now requires changes to proceed.Mar 9 2019, 3:46 PM
loh.tar updated this revision to Diff 53548.Mar 9 2019, 10:21 PM
loh.tar set the repository for this revision to R39 KTextEditor.
  • Add autoReloadThrottle
  • Add some var settings/signal which looks missed, pls compare to onModOnHdReload()
  • not sure if that static single shot make sense
  • pretty untested
dhaumann added inline comments.
src/document/katedocument.h
1086

What about this: make this a QAction with setCheckable(true). Then, you could reuse this action in the KTextEditor::Message. You can put this action then into the menu as well.

...then again, this could also be in tbe DocumentConfig just like many other settings we have.

loh.tar added inline comments.Mar 10 2019, 7:03 PM
src/document/katedocument.h
1086
  • That would be the first action member in katedocument
  • How that can add to the KTextEditor::Message didn't light me(?) Or why that would make sense

I'm working on a toggle action added to the View menu, so you can then enable that in advance. Surprising much effort :-/ but maybe I'm too stupid to do it in a simple way

loh.tar updated this revision to Diff 53613.Mar 10 2019, 7:22 PM
  • Add actions to View menu
  • Contains some garbage lines
  • Still poor tested
  • One issue may/is that in case of multible views/windows the actions only get synced after an reload event :-/

Just to be on the safe side: Does this also work, if you have two views visible showing the same document, and then the message appears? Does it also work, if you have two main windows showing the same document (View > New Window)?

src/view/kateview.cpp
238–239

Could you use new-style signal/slot syntax for new code? :)

Just to be on the safe side: Does this also work, if you have two views visible showing the same document, and then the message appears?

As said, poor tested., but what needs to be taken care? Normally these messages works well, so no idea why now not.

Does it also work, if you have two main windows showing the same document (View > New Window)?

Do you mean something else than this hint?

One issue may/is that in case of multible views/windows the actions only get synced after an reload event :-/

A tip how to solve this may helpful.

src/view/kateview.cpp
238–239

Yeah, normally I do. Here I was not sure if it will keeped/accepted and only did lazy copy old code :-)

BTW I'm surprised that such function (slot) didn't alrady exist. Instead will in the doc all views at some points directly called to update some things. See e.g. DocumentPrivate::documentReload:4310

loh.tar updated this revision to Diff 53891.EditedMar 14 2019, 3:38 PM
loh.tar set the repository for this revision to R39 KTextEditor.
  • Use new sig/slot style
  • Move/Use KToggleAction in document as suggested
  • Fix not working throttle logic
  • Increase delay to 3sec
  • Remove unneeded extra option "Follow"

TODO

  • Final position of action in menu. The still existing group is not needed anymore
  • Try it
#!/bin/bash
# Usage: ./me |tee -a foobar
while :
do
    echo $(date -Ins)
    # delay 1sec, -t 0.1 delay 100msec
    read -t 1 -n 1
done

I still need to test that again ;=)

cullmann accepted this revision.Sun, Mar 31, 10:18 AM

I think this works well enough to be added.
Could the "view_auto_follow" be implemented in a second review after this is commited?
I see there is some unresolved comment about action reuse, but I have no issue with this being commited as-is.
The feature is quiet nifty at least for "smaller" log files.

This revision is now accepted and ready to land.Sun, Mar 31, 10:18 AM

Could the "view_auto_follow" be implemented in a second review after this is commited?

I had remove this because it's somehow unhandy. When enabled you can't scroll up an look at some line without to be interrupted on next update. Now it works the smart way.

I see there is some unresolved comment about action reuse, but I have no issue with this being commited as-is.

Only these "Done" button was not klicked by me, but I think it's fixed as suggested by @dhaumann

Hmm, if the follow_.. stuff is removed, I think the sub-menu should go away, too.
The view_auto_reload should just be a top-level action, menus with single entries are strange ;)
Feel free to commit this with such a change.

loh.tar updated this revision to Diff 55199.Mon, Apr 1, 3:19 PM
  • Ensure the view jumps not back when user scrolls around
  • Don't reload while user scrolls
  • Fix missing connect to auto reload slot when enabled by modOnHdHandler
  • Change menu

As you see, not only the menu is changed, so I update this diff for your approval.

I think this works well enough to be added.

Harr, I found it now not good enough and spend some more time for this. Looks now nicely to me. The view still jumps slightly around but that may be go away by D17857, but not tested.

cullmann accepted this revision.Tue, Apr 2, 5:11 AM

The menu is now ok and the feature still works for me.
And yes, the view still jumps a bit.
:P Funny enough for me it failed during testing more because of missing file notifications, should not have tried it on NFS.

This revision was automatically updated to reflect the committed changes.