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

[loh tar]

This revision was automatically updated to reflect the committed changes.
Closed by commit R39:81a8d573ecaf: DocumentPrivate: Add option Auto 
Reload Document to View menu (authored by loh.tar).

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19517?vs=55199=55294

REVISION DETAIL
  https://phabricator.kde.org/D19517

AFFECTED FILES
  src/data/katepart5ui.rc
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h
  src/document/katedocument.cpp
  src/document/katedocument.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[Christoph Cullmann]

cullmann accepted this revision.
cullmann added a comment.


  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.

REVISION DETAIL
  https://phabricator.kde.org/D19517

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[loh tar]

loh.tar added a comment.


  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.

REVISION DETAIL
  https://phabricator.kde.org/D19517

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[loh tar]

loh.tar updated this revision to Diff 55199.
loh.tar added a comment.


  - 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
  
  F6742373: 1554131807.png 

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19517?vs=53891=55199

REVISION DETAIL
  https://phabricator.kde.org/D19517

AFFECTED FILES
  src/data/katepart5ui.rc
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h
  src/document/katedocument.cpp
  src/document/katedocument.h
  src/view/kateview.cpp
  src/view/kateview.h
  src/view/kateviewinternal.cpp

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[Christoph Cullmann]

cullmann added a comment.


  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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19517

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[loh tar]

loh.tar added a comment.


  > 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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19517

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[Christoph Cullmann]

cullmann accepted this revision.
cullmann added a comment.
This revision is now accepted and ready to land.


  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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19517

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[Christoph Cullmann]

cullmann added a comment.


  I still need to test that again ;=)

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19517

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[loh tar]

loh.tar updated this revision to Diff 53891.
loh.tar set the repository for this revision to R39 KTextEditor.
loh.tar added a comment.


  - 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
1. Usage: ./me |tee -a foobar while : do echo $(date -Ins)
  1. delay 1sec, -t 0.1 delay 100msec read -t 1 -n 1 done

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19517?vs=53613=53891

REVISION DETAIL
  https://phabricator.kde.org/D19517

AFFECTED FILES
  src/data/katepart5ui.rc
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h
  src/document/katedocument.cpp
  src/document/katedocument.h
  src/view/kateview.cpp
  src/view/kateview.h

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[loh tar]

loh.tar added a comment.


  > 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.

INLINE COMMENTS

> dhaumann wrote in kateview.cpp:238-239
> Could you use new-style signal/slot syntax for new code? :)

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

REVISION DETAIL
  https://phabricator.kde.org/D19517

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[Dominik Haumann]

dhaumann added a comment.


  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)?

INLINE COMMENTS

> kateview.cpp:238-239
>  
> +connect(m_doc, SIGNAL(reloaded(KTextEditor::Document*)), 
> SLOT(slotDocumentReloaded()));
> +connect(m_doc, SIGNAL(aboutToReload(KTextEditor::Document*)), 
> SLOT(slotDocumentAboutToReload()));
> +

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

REVISION DETAIL
  https://phabricator.kde.org/D19517

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[loh tar]

loh.tar updated this revision to Diff 53613.
loh.tar added a comment.


  - 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 :-/

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19517?vs=53548=53613

REVISION DETAIL
  https://phabricator.kde.org/D19517

AFFECTED FILES
  src/data/katepart5ui.rc
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h
  src/document/katedocument.cpp
  src/document/katedocument.h
  src/view/kateview.cpp
  src/view/kateview.h

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[loh tar]

loh.tar added inline comments.

INLINE COMMENTS

> dhaumann wrote in 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.

- 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

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19517

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> katedocument.h:1086
>  bool m_modOnHd = false;
> +bool m_autoReloadMode = false;
> +QTimer m_autoReloadThrottle;

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.

REPOSITORY
  R39 KTextEditor

REVISION DETAIL
  https://phabricator.kde.org/D19517

To: loh.tar, #ktexteditor, cullmann
Cc: dhaumann, cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, 
gennad, domson, michaelh, ngraham, bruns, demsking, sars

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

[loh tar]

loh.tar updated this revision to Diff 53548.
loh.tar set the repository for this revision to R39 KTextEditor.
loh.tar added a comment.


  - 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

REPOSITORY
  R39 KTextEditor

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19517?vs=53296=53548

REVISION DETAIL
  https://phabricator.kde.org/D19517

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h
  src/document/katedocument.cpp
  src/document/katedocument.h

To: loh.tar, #ktexteditor, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, 
michaelh, ngraham, bruns, demsking, sars, dhaumann

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

[Christoph Cullmann]

cullmann requested changes to this revision.
cullmann added a comment.
This revision now requires changes to proceed.


  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

REVISION DETAIL
  https://phabricator.kde.org/D19517

To: loh.tar, #ktexteditor, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, 
michaelh, ngraham, bruns, demsking, sars, dhaumann

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

[loh tar]

loh.tar updated this revision to Diff 53296.
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.
loh.tar edited the test plan for this revision.
loh.tar added a comment.


  - 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

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D19517?vs=53132=53296

REVISION DETAIL
  https://phabricator.kde.org/D19517

AFFECTED FILES
  src/dialogs/katedialogs.cpp
  src/dialogs/katedialogs.h
  src/document/katedocument.cpp
  src/document/katedocument.h

To: loh.tar, #ktexteditor, cullmann
Cc: cullmann, kwrite-devel, kde-frameworks-devel, #ktexteditor, gennad, domson, 
michaelh, ngraham, bruns, demsking, sars, dhaumann