Bug 156732

Created attachment 23298 [details]
A (sort of hacky) patch

A patch to do it. Sort of. :-) I don't know how to make cmake install stuff or
how to do paths cleanly, etc.

Also, is there a way to make TerminalInterface (or whatever) set an environment
variable? It might be better to make a folder with dolphin shell utils and add
that to $PATH, and then stuff in the mainwindow id into another environment
variable. Although, I'm not sure if their inheritability could cause oddness.

Incidentally, why does the Konsole KPart's implementation of
TerminalInterface::showShellInDir not follow the documentation... it doesn't
send "cd ..." if the shell is already running.

Depends on Bug #156919

*** Bug 167211 has been marked as a duplicate of this bug. ***

*** Bug 190940 has been marked as a duplicate of this bug. ***

*** Bug 221341 has been marked as a duplicate of this bug. ***

The reporter of bug 221341 suggested to watch /proc/$PID/cwd, where $PID is the process ID of the shell that belongs to the terminal panel.

*** Bug 273674 has been marked as a duplicate of this bug. ***

*** Bug 294839 has been marked as a duplicate of this bug. ***

Now that bug 156919 has been implemented, it should be possible for dolphin to implement the synchronization in the other direction. Just listen to the currentDirectoryChanged(const QString& dir) signal emitted from konsolepart.

Thanks Jekyll for the update, I'll try to use the new API (hopefully I can do it until 4.9)

Created attachment 70353 [details]
Patch (still work in progress)

@Jekyll: nice work :-)

This patch is a first step toward making use of the new konsole API. Some remarks about it:

1. I think it's necessary to keep track of the current directory of the konsole part to prevent sending an unneeded "cd" to the terminal after a currentDirectoryChanged(QString) signal is received and the URL is changed -> added a new member m_konsolePartCurrentDirectory.

2. When the terminal's current directory changes and the view's URL is updated accordingly, the terminal loses focus at the moment. I think that this is very distracting -> needs more work.

3. Sometimes, there is a noticeable delay between the "cd" command which is entered in the terminal and the update of the view's URL. I have the feeling that this delay originates inside the terminal, i.e., that the currentDirectoryChanged(QString) signal is not emitted right after the "cd" command has been entered, but a little while later.

Thanks Frank! Regarding point 2 (focus): I'd say don't lets touch the focus-handling in the class DolphinView - grabbing the focus when changing a directory is usually the preferred behavior and a good default. Probably introducing a kind of slot DolphinMainWindow::slotTerminalDirectoryChange() (naming?) might make sense: Here we can call changeUrl() and return the focus to the terminal-panel afterwards. This is just a suggestion, probably you have a better idea.

Great to hear that this is going to be implemented.

With respect to the question about focus: like Frank, I would prefer that the focus stays in the terminal if I change the directory via 'cd' in the terminal. This would allow to use dolphin as a kind of terminal replacement with the folder view as graphical upgrade.

@Frank:

Thanks for your work!

The delay is mentioned in the konsolepart commit[1]. That is also true for standalone Konsole .  Currently Konsole gets process information through polling /proc periodically and when needed (after keyboard event). For the concern of efficiency and load, the polling does not start immediately after each keystroke. A hardcoded 1000ms oneshot timer is used to start the polling later. If the user feedback suggests that delay is a show stopper for this new dolphin feature, I think it should be OK to decrase that 1000ms to a smaller value. But it is unlikely to be immediate.

As for the focus problem, I think that depends upon how (most) users take advantage of this new synchronization. 

   * some users think using "cd /some/path/" in the shell is the more efficient way than using mouse to enter into the needed folder, and they want to use dolphin to operate files after entering into that folder. So changing focus should make sense for them.  For them, they are using "dolphin + shell"

   * some users realize they can now use dolphin as an previewer or helper for their favorite ncurses filemanagers, like mc, vifm, and ranger. Most of time they want to do their work in the ncurses filemanagers running in the terminal panel. So chaning focus would totally defeat that possiblily. For them, they are using "mc/ranger + dolphin"

[1] http://commits.kde.org/konsole/675dbcb2fe777b22044510579321148989d29cfb

(In reply to comment #14)
> The delay is mentioned in the konsolepart commit[1]. 

I must have missed that in the commit message. Thanks for the detailed explanation! Just a quick idea: maybe one could reduce the 1000ms if the key pressed is "Enter"?

> As for the focus problem, I think that depends upon how (most) users take
> advantage of this new synchronization. 

It's also important to consider how any changes can break existing use patterns. I sometimes use the Terminal Panel to run some quick commands in the current folder, then realise I have to do something else in a subfolder, so I enter "cd subfolder" and run some more commands. In this situation, it would be unwanted to change focus.

I understand that changing focus may make sense in some use cases, but it would be a behaviour change that is likely to frustrate users who got used to the terminal *not* losing focus after a "cd" command. I'm pretty sure that I'm not the only one ;-)

(In reply to comment #12)
> Thanks Frank! Regarding point 2 (focus): I'd say don't lets touch the
> focus-handling in the class DolphinView - grabbing the focus when changing a
> directory is usually the preferred behavior and a good default. Probably
> introducing a kind of slot DolphinMainWindow::slotTerminalDirectoryChange()
> (naming?) might make sense: Here we can call changeUrl() and return the
> focus to the terminal-panel afterwards.

I tried that:

void DolphinMainWindow::slotTerminalDirectoryChanged(const KUrl& url)
{
    QWidget* focusWidget = QApplication::focusWidget();
    changeUrl(url);
    if (focusWidget) {
        focusWidget->setFocus();
    }
}

This does not work because DolphinViewContainer requests the focus asynchronously when the URL navigator's url changes, see DolphinViewContainer::slotUrlNavigatorLocationChanged(const KUrl& url).

The cleanest solution I can see at the moment is to add a function like DolphinViewContainer::doNotRequestFocusAfterNextUrlChange(), which could be called by DolphinMainWindow::slotTerminalDirectoryChanged(). An alternative could be that DolphinViewContainer tries to find out which widget has the focus and does not change focus if a parent of that widget is a TerminalPanel (using parent->metaObject()->className()), but this seems a bit more hackish to me.

> The cleanest solution I can see at the moment is to add a function like 
> DolphinViewContainer::doNotRequestFocusAfterNextUrlChange(), which
> could be called by DolphinMainWindow::slotTerminalDirectoryChanged().
> An alternative could be that DolphinViewContainer tries to find out which
> widget has the focus and does not change focus if a parent of that widget is a
> TerminalPanel (using parent->metaObject()->className()), but this seems
> a bit more hackish to me.

Hm... As you say checking the class-name inside DolphinViewContainer is a hack, let us ignore this ;-) In opposite to having a temporary state change due to an interface like DolphinViewContainer::doNotRequestFocusAfterNextUrlChange(), what do you think about adding something explicit like:
  /**
   * If \a grab is set to true, the container automatically grabs the focus
   * as soon as the URL has been changed. Per default the grabbing
   * of the focus is enabled.
   */
  void DolphinViewContainer:: setAutoGrabFocus(bool grab);
  bool DolphinViewContainer::autoGrabFocus() const;

DolphinMainWindow (which is aware about the terminal-panel + container) takes care to disable the autograbbing in case if the Terminal has the focus. Personally I'd prefer this interface in comparison to such internal state-changes like doNotRequestFocusAfterNextUrlChange(), which depends on triggering another action to get back the old behavior.

BTW: Generally I agree that we should not change the focus. We did not change the focus when synchronizing the terminal with the view and I think we should not change the focus too when synchronizing the view with the terminal. Of course as Jekyll said there might be usecases where such an automatic focus-change might be fine, but my gut feeling believes that these are corner-cases...

I myself prefer not changing focus, because I'm really interested with using "ranger + dolphin" . That is what is in my mind when I decide to add that konsolepart signal :)

My point is this new synchronization itself introduces behavior change, and some users might feel uncomfortable with it.

There is another minor problem with the current patch:  after hiding the terminal panel with F4, dolphin automatically sends "cd /"  into the shell, which makes terminal panel emit that directory changing signal later,  and finally dolphin shows the "/" folder. I guess that "cd /" is to make sure the shell does not stands in the way of unplugging removable media.  I would suggest disconnecting that signal after terminal panel is hidden and connecting it when the terminal panel is shown again later.

(In reply to comment #18)
> My point is this new synchronization itself introduces behavior change, and
> some users might feel uncomfortable with it.

You're right of course, any behaviour change may turn out to be unwanted for some users. But I can't see any way to prevent this here. IMHO, the benefit of syncing the view's URL with the terminal is much larger than the risk of a large number of users not liking it. And those users still have the possibility to work in an external Konsole by pressing Shift+F4.

> There is another minor problem with the current patch:  after hiding the
> terminal panel with F4, dolphin automatically sends "cd /"  into the shell,
> which makes terminal panel emit that directory changing signal later,  and
> finally dolphin shows the "/" folder. I guess that "cd /" is to make sure
> the shell does not stands in the way of unplugging removable media.  I would
> suggest disconnecting that signal after terminal panel is hidden and
> connecting it when the terminal panel is shown again later.

Thanks, I had already noticed that when playing around with it. I'll work on that.

Created attachment 70382 [details]
Updated patch

(In reply to comment #17)
> Hm... As you say checking the class-name inside DolphinViewContainer is a
> hack, let us ignore this ;-) 

I fully agree ;-) My new patch builds upon the DolphinViewContainer changes suggested by Peter and the connect/disconnect approach proposed by Jekyll. Note that I added a pointer to the Konsole part to TerminalPanel to make it possible to connect to/disconnect from the part's signal at any time. Casting from m_terminal is not possible via qobject_cast because m_terminal is not a QObject (a dynamic_cast would be possible, however).

I have not found any situation where it doesn't work as it should, but many eyes make bugs shallow ;-)

A (sort of off-topic) question that I asked myself when working on this: Does anyone remember why the "hide terminal" situation is handled in a slot (which has to be connected by DolphinMainWindow) rather than reimplementing hideEvent() and ignoring (i.e., forwarding) spontaneous events? That would be more symmetric to the showEvent() situation and also slightly reduce the complexity of DolphinMainWindow.

(In reply to comment #20)
> Created attachment 70382 [details]
> Updated patch

I have used the patched dolphon for a few days. No problems observed so far.

Git commit b53f7c6c19dda3cd1f11b9cb4c2aca84aa806fd1 by Frank Reininghaus.
Committed on 26/04/2012 at 08:31.
Pushed by freininghaus into branch 'master'.

Update the view when changing the directory using 'cd' in the terminal

Thanks to Jekyll Wu for helping to implement this feature!
FIXED-IN: 4.9.0

M  +8    -0    dolphin/src/dolphinmainwindow.cpp
M  +7    -0    dolphin/src/dolphinmainwindow.h
M  +13   -3    dolphin/src/dolphinviewcontainer.cpp
M  +9    -0    dolphin/src/dolphinviewcontainer.h
M  +34   -6    dolphin/src/panels/terminal/terminalpanel.cpp
M  +12   -0    dolphin/src/panels/terminal/terminalpanel.h

http://commits.kde.org/kde-baseapps/b53f7c6c19dda3cd1f11b9cb4c2aca84aa806fd1

I never thought that was a feature, i thought it was a bug, i was even going ot create a bug report about this, but then i was told that this was in fact some dolphin feature, so i asked if was possible to disable this and i also was told that atm that does not exist.

So i gently ask to dolphin developers to create a way to disable dolphin from follow konsole "movements", its rather annoying to have symlinks in out home dir and each time i click on them im automatically redirected to the original location , and there i go again move the mouse to go back to my home dir.

(In reply to comment #23)
> its rather annoying to have symlinks in out
> home dir and each time i click on them im automatically redirected to the
> original location , and there i go again move the mouse to go back to my
> home dir.

Of course this is a bug. I've created a new bug report about this (bug 302037). The right solution is to fix this bug, not to work around it by disabling the feature.