Bug 1677

Changed status NEW -> PATCHAVAIL

Btw, why can't a new bug be created with status PATCHAVAIL from the beginning?

I've just tested and it works well for me.
It's an interesting patch.

I tested the patch. Works well here. Simple and improves efficiency.

Before pushing to git, should we update the patch or create a second patch to add the respective changes to Konqueror -> Settings -> Configure Shortcuts?

Possibly:

Rename, move selector down    Tab
Rename, move selector up      Shift-Tab

Possibly update the Konqueror Help file too?

Option to set hotkeys looks like a good idea.

Besides, I intend to add constructors to maintain ABI compatibility.

I looked through the konqueror docbook sources and did not find an obvious place to add the new shortcut information. If we add the information to the Settings->Configure shortcuts dialog we should be okay.

I will rework the patch to:
1) add constructors to maintain ABI compatibility as suggested by Slavek
2) add shortcut support

Just an update.
I am reworking the patch. The part for tdelibs is ready, but I haven't got the time to do the part for tdebase yet. Should be ready sometime this week.
I have remove the changes to the constructors and instead use set methods for enabling/disabling the move-after-rename feature.

Created attachment 1575 [details]
tdelibs and tdebase patch

Finally the reworked patch is ready.
There are no changes to any constructor, now I just use setter methods.
The shortcuts can be set from Konqueror -> Configure Shortcuts.. -> Shortcut and they are called:
- Rename and move to the next item (default shortcut is Tab)
- Rename and move to the previous item (default shortcut is Shift+Tab)
They do not need to work together, you could for example set just one of the shortcut without using the other one.
The functionality is the same as described when I posted the original patch.

When an item is being renamed, the keyboard focus is on the TDEListViewLineEdit editor and some keys (for example Enter, Up, Down, Tab and Shift+Tab) are hard coded in its code. To make sure that those keys can be used for the 'move next/prev' functionality as well, I had to "abuse" a little the use of TDEAction. 
Normally, a TDEAction is connected to a slot and when the action is triggered, the connected function is invoked.
For this patch, I had to use two 'fake' TDEAction, so that the shortcuts could be set from Konqueror -> Configure Shortcuts.. -> Shortcut. The two actions themselves don't do anything at all (in fact they are disabled and disconnected).
When we start a rename operation, the associated shortcuts are passed to the TDEListViewLineEdit editor, which will detect if any of those shortcuts has been pressed and eventually emit the associated 'move next/prev' signal.

Michele, in the future would you please post multiple patches as individual diff files rather than combined into an archive? Easier to review online and easier to download to each respective build directory. Yeah, I know, that means two submissions to the bugzilla. Been there done that. :-)

I'll rebuild and test....

> Michele, in the future would you please post multiple patches as individual
> diff files rather than combined into an archive? Easier to review online and
> easier to download to each respective build directory. Yeah, I know, that means
> two submissions to the bugzilla. Been there done that. :-)

Okey-dokey, no problem. 
Actually it would be even better if it was possible to add multiple files in a single submission. Do you know if this is a bugzilla limitation or just a matter of settings?

>Do you know if this is a bugzilla limitation or just a matter of settings?
I don't know. Tim and Calvin were the Trinity bugzilla maintainers but Calvin hasn't been around in a long time. Multiple attachments with one submission would be nice, as well as being able to toggle PATCHAVAIL without a second submission.

Tagging multiple attachments as obsolete, as is the case after pushing to git, would be nice too. Instead attachments can only be tagged as such individually.

I also had some comments to the patches:

1) I believe that there is no need include changelog into modified files. I say this not because I wanted to disparage your good work, but changes are recorded in git commits.

2) The patches contained multiple changes of "white space". I accept that the goal is to make code cleaner, but the patches can then be larger => a bit more difficult for review before pushing.

With the latest patch, Shift-Tab doesn't work for me. Tab works fine.

* Press F2 to begin renaming a file.
* Do not rename the file.
* Press Tab to move the selection down to the next file.
* When reaching the last file in the list, the selection wraps to the first file in the list at the top.

When reversing the order with Shift-Tab, the renaming stops, and the selection moves to the tab, then to the Location bar, etc.

The shortcuts appear in the dialog, but in a section named 'Shortcuts'. The search feature in the dialog finds the new shortcuts and F2 is standard from long ago in the KDE 3 days. Yet I suspect most Trinity users know that shortcut. If the two new shortcuts were listed immediately after Rename then likely users would discover them more easily.

>The patches contained multiple changes of "white space".
I like stripping spaces too, but a long time back I realized how that creates havoc with patches. I have kate configured to strip white spaces while I have kwrite configured not to do that. When I create patches I use kwrite to avoid the problem. :-)

> 1) I believe that there is no need include changelog into modified files. I say
> this not because I wanted to disparage your good work, but changes are recorded
> in git commits.
No problem at all Slavek. I am used to do so where I work, but I can avoid that for TDE. You can remove all those changelog, even the one already committed to GIT

> 2) The patches contained multiple changes of "white space". I accept that the
> goal is to make code cleaner, but the patches can then be larger => a bit more
> difficult for review before pushing.
This is a delicate topic. In fact different editor settings (for example use tab or whitespace, tab width, strip whitespaces) can create those kind of "white space" in the patch and also other patch sections due to different indentation in the code.
I think it would be worth to discuss about a common editing style, so that whoever is looking at the code would have exactly the same code.
Where I work we follow a set of common guidelines, so that we all have the same style.

As for TDE patches, I will remove those whitespace from the .diff file before submitting it.

> The shortcuts appear in the dialog, but in a section named 'Shortcuts'. The
> search feature in the dialog finds the new shortcuts and F2 is standard from
> long ago in the KDE 3 days.
Yes, that would be good and was my original idea as well (and actually started coding for that). Unfortunately, it's not as straightforward as it looks like. I am not talking about code implementation (that can surely be done), but relation among the different parts of Konqueror and TDE as well.
The move and rename functionality (as some more) are implemented in the Konqueror listview module, which is compiled into a library. Therefore I believe it has been designed to be usable also independently, even though I can't be sure 100%.
Therefore I have followed the logic of the others listview actions and implemented the code for 'rename and move' to be fully contained inside the listview library, without creating an additional link between the listview and the main konqueror module.
That's the reason why those two actions appears in the Shortcut section, together with other actions that are contained in the listview module.

It's a little awkward for the user, but IMO it is better to keep the existing approach, without compromising module interdependency. 
What do you think?

(In reply to comment #13)
> With the latest patch, Shift-Tab doesn't work for me. Tab works fine.
> 
> * Press F2 to begin renaming a file.
> * Do not rename the file.
> * Press Tab to move the selection down to the next file.
> * When reaching the last file in the list, the selection wraps to the first
> file in the list at the top.
> 
> When reversing the order with Shift-Tab, the renaming stops, and the selection
> moves to the tab, then to the Location bar, etc.

Shift-Tab works on my system, so I need your help because I can't reproduce the problem on my machine. I will post some instructions a little later (maybe two or three hours later), then you will have to recompile tdelibs, install and test if you don't mind.
In the mean time, could you try a different key combination? An interesting one is 'Enter' and 'Shift+Enter' because 'Enter' is also hard coded in the TDELineEdit component.

Created attachment 1577 [details]
code for debug

> I will post some instructions a little later (maybe two
> or three hours later), then you will have to recompile tdelibs, install and
> test if you don't mind.
> In the mean time, could you try a different key combination? An interesting one
> is 'Enter' and 'Shift+Enter' because 'Enter' is also hard coded in the
> TDELineEdit component.

Darrell, please try this. After applying the previous patch and before compiling, open the file /tdelibs/tdeui/tdelistview.cpp, go to line 334 and replace the code for TDEListViewLineEdit::event and for TDEListViewLineEdit::keyPressEvent with the code in the attached file. 
It's exactly the same logic, but everytime you press a key it will print a debug message. 
Recompile tdelibs (actually I haven't recompile that code, but there should be no errors :) ) and install. You shouldn't need to recompile Konqueror.
Run Konqueror from the command line and try to rename a file. You should get a list of messages on the Konsole which contains the code of the key being pressed. You should be able to check what happens when Tab and Shift-Tab are pressed.
If possible write down the sequence of keys you press and post it together with the messages printed on Konsole.

Same behavior with a fresh profile: Shift-Tab fails.

BTW, my observations are with a full package rebuild.

>It's a little awkward for the user, but IMO it is better to keep the existing
>approach, without compromising module interdependency. What do you think?
I have no preference --- just sharing observations. Do what works best. :-)

>In the mean time, could you try a different key combination? An interesting one
>is 'Enter' and 'Shift+Enter' because 'Enter' is also hard coded in the
>TDELineEdit component.
Changing the shortcut: Enter and Shift+Enter both work as expected. Interesting.

Superfluous side note: Why does Trinity (nee KDE3) use the term 'Return' rather than 'Enter'? All of my keyboards have 'Enter' stamped on the key, not 'Return'.

I have not had time to test the debug patch.

I applied the temporary debug patch.

I selected a file by single-clicking on the file with the mouse pointer. I then pressed F2 to begin renaming. I did not rename the file. Then three times I pressed Tab and then pressed Esc. The results:

DBG: TDEListViewLineEdit::event() invoked 4097
DBG: TDEListViewLineEdit::keyPressEvent() invoked 4097
DBG: using move on rename
DBG: moving next
DBG: TDEListViewLineEdit::event() invoked 4097
DBG: TDEListViewLineEdit::keyPressEvent() invoked 4097
DBG: using move on rename
DBG: moving next
DBG: TDEListViewLineEdit::event() invoked 4097
DBG: TDEListViewLineEdit::keyPressEvent() invoked 4097
DBG: using move on rename
DBG: moving next
DBG: TDEListViewLineEdit::event() invoked 4096
DBG: TDEListViewLineEdit::keyPressEvent() invoked 4096

Next I selected the same file by single-clicking on the file with the mouse pointer. I then pressed F2 to begin renaming. I did not rename the file. Then three times I pressed Shift+Tab and then pressed Esc. The results:

DBG: TDEListViewLineEdit::event() invoked 4128
DBG: TDEListViewLineEdit::keyPressEvent() invoked 4128
DBG: TDEListViewLineEdit::event() invoked 4098

Notice the absence of the "DBG: moving prev" message.

Created attachment 1585 [details]
code for debug 2

Thanks for your help Darrell.
The fact that Enter and Shift+Enter works fine seems to point to a problem with how Shift+Tab is handled or with the shortcut settings.
This days I am quite busy coding other stuff that I must finish within next Tuesday, so I can't work on this too much. Anyhow could you do the following test as well?

1) Try the code in the new attachment. It's basically the same but it also prints the settings for the shortcuts passed to the TDELineEdit component. Keep in mind that Tab and Shift+Tab will generate an Event() for some reasons, while other keys will generate a KeyPressEvent()

2) Also you could try to invert Tab and Shift+Tab shortcuts and see what happens. This is actually a good test to do

Note: I tried to compile the same code on my machine today, but I got an error towards the end (after the actual compilation) for an icon file missing. As I said above, I am coding other stuff, so for a few days I can only rely on your feedback for lack of time.

Something strange happening here.

Yesterday when I swapped Tab and Shift+Tab the shortcut read 'Tab' for both. Today I tried swapping and now they say Tab and Shift+BackTab. And reverse renaming/moving now works.

I haven't figured out what is happening or why the change.

I observe the following:

* Change Move Next from Tab to None
* Change Move Previous from Shift+Tab to Tab
* Change Move Next from None to Custom and press Shift+Tab

Both directions work as expected.

Change both shortcuts to the Default.

Move Previous stops working.

Now the twist:

* Change Move Next to Default
* Change Move Next to Custom and press Shift+Tab

Notice Move Previous gets displayed as Shift+Backtab and not Shift+Tab.

Seems the problem is Tab and Backtab are not being interpreted the same. I have no idea what Backtab is anyway. Backtab sounds like Shift+Tab.

When I use Custom and press Shift+Tab, which displays as Shift+Backtab, my user profile ~/.trinity/share/apps/konqlistview/konq_treeview.rc is modified to add the following:

 <ActionProperties>
  <Action shortcut="Shift+Backtab" name="renameMovePrev" />
 </ActionProperties>

When I set Move Previous to Default, which displays as Shift+Tab, the previous ActionProperties is removed.

Created attachment 1589 [details]
new tdebase patch. the tdelibs part remains unchanged

Hi Darrell,
thanks for your good work. I have finally found were the problem is.
In my settings, I had the 'move prev' shortcut set to Custom + Shift+Backtab (because I changed that several times while developing and testing the patch) and that's why it was working on my system. As soon as I changed that back to Default, I could reproduce the same behavior that you discovered.

BackTab is the key raised by QT when Shift+Tab is pressed (see http://trinitydesktop.org/docs/qt3/qt.html#Key-enum for the key list, Tab and BackTab are right at the top). 
TDE shortcut sequences make use of key modifiers such as Ctrl, Shift and Alt, so in the code that created the two TDEActions, I used Shift+Tab for 'move prev'. That in my mind should have been the same as BackTab, similar to what would be for example for Enter and Shift+Enter (or any other key).
For some obscure reasons though, it turns out that Shift+Tab is not the same as Shift+BackTab, either this is intentional (?) or just a bug.
So I just changed Shift+Tab to Shift+BackTab in the code that creates the 'move prev' shortcut and now the problem is gone.

Attached the new tdebase patch, where I also removed the additional comments at the beginning of the modified files and all the meaningless white space changes (I kept a couple of them though, that I think make sense).
The tdelibs part remains the same, but I will post a new version soon without additional comments and whitespaces

Created attachment 1590 [details]
the new tdelibs patch

I'll test the updated patch.

Regarding Key_Backtab, the terminology is confusing. Backtab == Shift+Tab. Users will be familiar with the terminology of Tab abd Shift+Tab, not Tab and Shift+Backtab. When users see Shift+Backtab that implies Shift+(Shift+Tab), which implies Tab. :-)

I understand at the code level using Key_Backtab, but at the dialog interface level with users that should be displayed as Shift+Tab. Is that possible?

If this was intentional then the original designers had their heads in the clouds. I'm more inclined to think this is a bug. Not your fault, of course. :-)

The latest patches are working fine here. Good job!

> I understand at the code level using Key_Backtab, but at the dialog interface
> level with users that should be displayed as Shift+Tab. Is that possible?
> If this was intentional then the original designers had their heads in the
> clouds. I'm more inclined to think this is a bug. Not your fault, of course.

Fully agree with you. Shift+BackTab is counter intuitive.
i have open a new bug report for this (bug 1695)

I remain confused (nothing new there!). :-)

In KControl, Keyboard Shortcuts, when I press Shift+Tab the UI displays Shift+Tab. Not Shift+Backtab.

Perhaps look at that code and update the patches?

Before the patches were updated Shift+Tab was working just fine.

> Before the patches were updated Shift+Tab was working just fine.

In the first version, the code for checking Tab and Shift+Tab was hard coded in the TDELineEdit component, and if you look you will see Qt::Key_Tab and Qt::Key_BackTab in the code

+    if (k->key() == Qt::Key_Backtab || k->key() == Qt::Key_Tab)
+    {
+      if (m_useRenameSignals)
+      {
+        keyPressEvent(k);
+        return true;
+      }

That's why it was working just fine :-)


> In KControl, Keyboard Shortcuts, when I press Shift+Tab the UI displays
> Shift+Tab. Not Shift+Backtab.
> Perhaps look at that code and update the patches?

This is interesting. From the Konqueror -> Configure Shortcut window instead, Shift+Tab is displayed as Shift+BackTab, so there is definitely something strange going on here. I wonder if this Shift+BackTab was not a bug after all.
I will add info to bug 1695.

The Shift+Tab/Shift+BackTab issue probably extends to most of TDE, not just this patch or Konqueror only. 
So IMO this patch could/should be pushed as it is at the moment.
When I have the time to look at bug 1695, I will convert Shift+BackTab to Shift+Tab in all the applicable points, including the code changed by this patch.

Patch for tdelibs pushed to GIT in hash 6bf8e926.

Before pushing patch for tdebase I'll wait a little for rebuild updated tdelibs on build-farm.

Created attachment 1603 [details]
Glitch in rendering focus

I tested enhanced renaming and I noticed glitch in rendering focus - see attached picture. Notice highlight color on file name, but also strip on the background of whole column with file name.

I don't see that on my system. I am using alternate shading. I forgot how to disable that so I can compare to your screenshot. How do I disable?

Now I have checked: this is not a bad rendering of focus => files are really added to the selection.

I will double checked when I am on a TDE machine. The original idea was to leave the selection unchanged, i.e. no adding or removing any file to the current selection.
Maybe when I reworked the patch something got lost, even though I don't remember seeing such behavior on my machine. I will confirm later today.

Slavek, I double checked on my TDE machine, and as well as Darrell I do not experience the problem  that you mentioned.
After the 'move next/prev' rename operation is completed/aborted, the selection is unchanged, which by the way was the original design idea.
I tried several times, each time with a random number of items selected (including the case with 0 and all items selected) and a random number of 'move next/prev'.

If you see the code in the patch file, in the functions 'KonqBaseListViewWidget::slotRenameNextItem' and 'KonqBaseListViewWidget::slotRenamePrevItem' I only set the current item ( setCurrentItem(nextItem);  or  setCurrentItem(prevItem); ) but I do not do any action on the current selection.

I am not sure why you see this behavior on your machine.
I believe it is not created by this patch for the reasons described above, but that more likely this patch exposes another problem that it what you have noticed.

I have some additional observations.
Try the following:

1) Open the folder
2) Use (arrow) keys to move to the file from where you want to start renaming. Whether it is a different file than the first in a list
 => file is automatically selected.
3) Press F2 to start renaming
4) Press Tab repeatedly to move to the next files
 => notice that the selection extends over all concerned files.

It seems that the behavior depends on fact how the file where renaming starts become part of the selection.

Patch for tdebase pushed to GIT in hash b42f6dbb.

> 1) Open the folder
> 2) Use (arrow) keys to move to the file from where you want to start renaming.
> Whether it is a different file than the first in a list
>  => file is automatically selected.
> 3) Press F2 to start renaming
> 4) Press Tab repeatedly to move to the next files
>  => notice that the selection extends over all concerned files.
> It seems that the behavior depends on fact how the file where renaming starts
> become part of the selection.

Hi Slavek, I did a lot of testing and I can replicate the behavior you have seen as well. I observed that there are two different situations.

1) moving with the arrow keys when not renaming, select the next/prev item in the list. In this situation using Tab/Shift+Tab while renaming extends the selection as you observed

2) moving with the arrow keys when not renaming, does NOT select the next/prev item in the list. In this situation using Tab/Shift+Tab while renaming does NOT extends the selection as I observed. For example, this seems to always happen in the case when more than 1 file is selected and we use the arrows for moving around.

The strange thing is that I can not find a constant pattern during my testing. It seems that sometimes we are in situation 1 and sometimes in situation 2, but even repeating the same sequence of actions at two different times, sometimes has the same effect and sometimes it work in the other situation.

Basically when moving using arrows, situation 1 is a "select item on move", while situation 2 is "move but do not select item". If we start from situation 1 when renaming, we have an extension of the selection. If we start from situation 2 when renaming, the selection remains unchanged.
This at least is consistent with the patch, since the code does not do any action on the selection itself.

These two situations exists regardless of this patch, since the two different behaviors can be observed even without renaming any item but just moving around with the arrows.

I suggest we close this bug since the patch itself if not responsible for what we have observed and eventually open a new bug like "Konqueror listview item selection when moving using arrows is inconsistent".
What do you think?

Created attachment 1612 [details]
Example of rename without extension (situation 2)

(In reply to comment #41)
> Created attachment 1612 [details]
> Example of rename without extension (situation 2)

The example is done using two times 'arrow down', then two times 'tab' and finally 'Enter'.

Need to observe that sometimes when using 'arrow down/up' the selection is changed as well (situation 1) and sometimes not (situation 2)

Created attachment 1613 [details]
Fix unintended automatic selection during renaming in Konqueror listview

I searched inside the methods that are responsible for the selections and it seems that I have a quite simple solution.

Please test it also on your machine.

(In reply to comment #43)
> Created attachment 1613 [details]
> Fix unintended automatic selection during renaming in Konqueror listview
> 
> I searched inside the methods that are responsible for the selections and it
> seems that I have a quite simple solution.
> 
> Please test it also on your machine.

Tested and works well. This resolves the problem with the rename, but the original 'strange' behavior is still there.
I think you can push the modified patch as well, and then if you think it is necessary, open a separate bug report for the original 'strange' behavior of Konqueror

Unintended automatic selection fixed in GIT hash 0bc7a6a5.

Is it possible to observe strange behaviour with the selection also in situation other than renaming?

> Is it possible to observe strange behavior with the selection also in
> situation other than renaming?

Yes, as I explained in comment 40, the strange behavior with selection happens also without doing any renaming. When moving using the arrows, sometimes the item is selected, sometimes not.
That's why I suggested to open a new bug report for "Konqueror listview item
selection when moving using arrows is inconsistent"

I am closing this bug as resolved since the rename functionality has been fully implemented in the listview.
I have opened bug 1740 for the inconsistent selection behavior using the keyboard