In D10990#219907, @ngraham wrote:

The patch no longer applies for me on master

Created and checked out branch arcpatch-D10990.


    This diff is against commit 48b58f830a585b773435c9af5ee2fe8f0c7c641d, but
    the commit is nowhere in the working copy. Try to apply it against the
    current working copy state? (436ad965e93409e2225b3d0451e997fb655b3e87)
    [Y/n] y

Checking patch src/dolphinmainwindow.cpp...
error: while searching for:
#include <KFilePlacesModel>
#include <KHelpMenu>
#include <KIO/JobUiDelegate>
#include <KJobWidgets>
#include <KLocalizedString>
#include <KMessageBox>

error: patch failed: src/dolphinmainwindow.cpp:51
Hunk #2 succeeded at 1213 (offset -2 lines).
Checking patch src/dolphincontextmenu.cpp...
Hunk #1 succeeded at 195 (offset 3 lines).
Hunk #2 succeeded at 312 (offset 3 lines).
Applying patch src/dolphinmainwindow.cpp with 1 reject...
Rejected hunk #1.
Hunk #2 applied cleanly.
Applied patch src/dolphincontextmenu.cpp cleanly.

 Patch Failed! 
Usage Exception: Unable to apply patch!

It works fine for me:

$ arc patch  D10990
Created and checked out branch arcpatch-D10990.     
Checking patch src/dolphinmainwindow.cpp...   
Checking patch src/dolphincontextmenu.cpp...                                  
Applied patch src/dolphinmainwindow.cpp cleanly.      
Applied patch src/dolphincontextmenu.cpp cleanly.                                                            
 OKAY  Successfully committed patch.

It works for you because the missing commit is in your local checkout's history, but not in the remote repo. Please reset your local master branch (git reset --hard origin/master) and then re-base again. This is why it's best not to make commits on master even in your local checkout.

Thanks!

So what do you think about adding Show Original into the File menu when a shortcut is selected?

Also, I notice that the behavior is still different for files and folders: original files are highlighted in their parent directories, while original folders are opened. Little inconsistencies like this tend to drive OCD people mad. :)

In D10990#220200, @ngraham wrote:

So what do you think about adding Show Original into the File menu when a shortcut is selected?

I'm positive about this. But I think this should be done after unifying file menu and context menu (Compress, Actions, etc. is not shown in the file menu).

Also, I notice that the behavior is still different for files and folders: original files are highlighted in their parent directories, while original folders are opened. Little inconsistencies like this tend to drive OCD people mad. :)

That's strange behavior, but in my opinion, it's OK for a such an infrequent feature as show original.

In D10990#220203, @rominf wrote:

In D10990#220200, @ngraham wrote:

So what do you think about adding Show Original into the File menu when a shortcut is selected?

I'm positive about this. But I think this should be done after unifying file menu and context menu (Compress, Actions, etc. is not shown in the file menu).

Why not do it now? We're already touching this code, might as well do the right thing here to set a good example. If we don't, we're just making that problem worse and giving ourselves more work later, no?

Also, I notice that the behavior is still different for files and folders: original files are highlighted in their parent directories, while original folders are opened. Little inconsistencies like this tend to drive OCD people mad. :)

That's strange behavior, but in my opinion, it's OK for a such an infrequent feature as show original.

If it's strange behavior, we should fix it. :) Using KIO:highlightInFileManager() would resolve the situation, as this is exactly what it was designed for. Opening a new window isn't the end of the world, and probably beneficial anyway for normal users who might otherwise get confused when the current view abruptly changes to a different one.

And if we also fix https://bugs.kde.org/show_bug.cgi?id=183429, then the problem basically goes away entirely. It's generally better to fix underlying issues if we can, rather than working around them.

Opening this Diff for the first time I was quite skeptical too, but as I actually needed this function recently and it is only added for links, I do think it could be quite useful to have.

In D10990#220200, @ngraham wrote:

Also, I notice that the behavior is still different for files and folders: original files are highlighted in their parent directories, while original folders are opened. Little inconsistencies like this tend to drive OCD people mad. :)

Are you sure? For me it is working, as Roman is already using KIO::highlightInFileManager. Also I like how this keeps both link and target in view, instead of just going to another location or hiding one item in a tab.

In D10990#220271, @rkflx wrote:

Opening this Diff for the first time I was quite skeptical too, but as I actually needed this function recently and it is only added for links, I do think it could be quite useful to have.

In D10990#220200, @ngraham wrote:

Also, I notice that the behavior is still different for files and folders: original files are highlighted in their parent directories, while original folders are opened. Little inconsistencies like this tend to drive OCD people mad. :)

Are you sure? For me it is working, as Roman is already using KIO::highlightInFileManager. Also I like how this keeps both link and target in view, instead of just going to another location or hiding one item in a tab.

Ah, you're right. Perhaps I've exposed a bug in KIO::highlightInFileManager() or how it's being called, because for most directory symlinks, it works as expected, but not for this one that I have:

$ ls -la ~/nate_shared
lrwxrwxrwx 1 dev dev 15 Feb  1 21:10 /home/dev/nate_shared -> /media/sf_nate/

Invoking Show Original on ~/nate_shared opens a new window showing the contents of /media/sf_nate instead of opening /media and highlighting sf_nate.

Bug in KIO::highlightInFileManager(), or a bug in the implementation here?

In D10990#220293, @ngraham wrote:

Ah, you're right. Perhaps I've exposed a bug in KIO::highlightInFileManager() or how it's being called, because for most directory symlinks, it works as expected, but not for this one that I have:

$ ls -la ~/nate_shared
lrwxrwxrwx 1 dev dev 15 Feb  1 21:10 /home/dev/nate_shared -> /media/sf_nate/

Invoking Show Original on ~/nate_shared opens a new window showing the contents of /media/sf_nate instead of opening /media and highlighting sf_nate.

Bug in KIO::highlightInFileManager(), or a bug in the implementation here?

This is easy to reproduce:

mkdir a
ln -s a link1
ln -s a/ link2
ls -la

Dolphin creates the first kind of link, which works fine. The other type was probably created by other means.

Is it a bug in KIO? Not really. As a developer, I want to be able to decide for myself what I want to open: Either the folder itself, or the parent folder with the subfolder highlighted. We could change KIO to strip the / and add a parameter for the behaviour, but that's quite complicated and in most cases the developer has to decide anyway how to handle the situation.

@rominf Here we could just use QUrl::StripTrailingSlash.

Also, there is still something odd: For me the patch even wants to open a/a instead of a and obviously fails for that. Adding qDebug(), we get QUrl("file:a/"), meaning the handling of relative symlinks is broken in this patch.

In D10990#220215, @ngraham wrote:

In D10990#220203, @rominf wrote:

In D10990#220200, @ngraham wrote:

So what do you think about adding Show Original into the File menu when a shortcut is selected?

I'm positive about this. But I think this should be done after unifying file menu and context menu (Compress, Actions, etc. is not shown in the file menu).

Why not do it now? We're already touching this code, might as well do the right thing here to set a good example. If we don't, we're just making that problem worse and giving ourselves more work later, no?

I don't understand the logic supporting main menus: Rename, Move to Trash, Delete items are located in File menu, while Cut, Copy, Paste are located in Edit menu; New Tab is in the File menu, while Recently closed tabs is in the Go menu. I think this should be refactored. On the other hand, menu is not shown be default. Is it worth to have it, especially in this state?

I'd say fiddling with the menu bar should be done in a follow-up patch, to keep this Diff manageable. In principle Nate is right, though, those actions should also be available in the menu.

I also find a bug (?) in KIO::highlightInFileManager().

Steps:

1. Open Dolphin
2. Navigate to appropriate dir
3. Press Show Original on the symlink to some hidden file
4. Observe Dolphin, that is opened in a right folder. But the required item is not highlighted because it's hidden.

Should we enable "Show hidden files" in this case?

In D10990#220532, @rominf wrote:

I also find a bug (?) in KIO::highlightInFileManager().

Steps:

1. Open Dolphin
2. Navigate to appropriate dir
3. Press Show Original on the symlink to some hidden file
4. Observe Dolphin, that is opened in a right folder. But the required item is not highlighted because it's hidden.

Nice catch, that's how thorough testing should be done…

Should we enable "Show hidden files" in this case?

Tough question. What should not be done is actually changing the config of Dolphin (global or only for a specific folder) without the user actively initiating this action. You could try to find a way to temporarily enable hidden files. If that does not work out or is too ugly, perhaps just show a message to the user.

@ngraham, please review

Yes, I agree with @rkflx that it would be nice if we can find a way to temporarily show hidden files to reveal the original.

As for the symlink-following behavior, here are the results of my tests:

• File absolute symlink: pass
• File relative symlink: pass
• Directory absolute symlink with trailing slash: pass
• Directory absolute symlink without trailing slash: pass
• Directory relative symlink with trailing slash: pass
• Directory relative symlink without trailing slash: pass
• Broken link: unclear: opens the directory that formerly contained the missing item; maybe it should warn instead (see below) and not open anything?
• Link to file that's inaccessible (e.g on unavailable network share or external disk): needs work: nothing happens. Maybe let's show a KMessageWidget saying "Could not access <missing path>" when this happens.

Are you going to do a follow-up patch to add the missing items to the menus, @rominf?

In D10990#221691, @ngraham wrote:

Yes, I agree with @rkflx that it would be nice if we can find a way to temporarily show hidden files to reveal the original.

I think that it would be even better if we show only required the hidden file, but not all. It might be a lot of work though.

As for the symlink-following behavior, here are the results of my tests:

• Broken link: unclear: opens the directory that formerly contained the missing item; maybe it should warn instead (see below) and not open anything?
• Link to file that's inaccessible (e.g on unavailable network share or external disk): needs work: nothing happens. Maybe let's show a KMessageWidget saying "Could not access <missing path>" when this happens.

Fixed, please check.

Are you going to do a follow-up patch to add the missing items to the menus, @rominf?

To be honest, I'm not interested because I don't use the menu as most of the Dolphin users I guess. I don't use a menu because it's hidden by default, I like control button more and file menu is bad structured and scanty.

In D10990#221801, @rominf wrote:

Are you going to do a follow-up patch to add the missing items to the menus, @rominf?

To be honest, I'm not interested because I don't use the menu as most of the Dolphin users I guess. I don't use a menu because it's hidden by default, I like control button more and file menu is bad structured and scanty.

OK, then can you add the new Show Original item to the File menu in this patch? Let's not make things even worse in patches where we add something to the context menu.

Maybe we should even add it to the Control menu too, as it's contextual and won't appear there all the time, so it won't amount to much clutter.

In D10990#221937, @ngraham wrote:

In D10990#221801, @rominf wrote:

Are you going to do a follow-up patch to add the missing items to the menus, @rominf?

To be honest, I'm not interested because I don't use the menu as most of the Dolphin users I guess. I don't use a menu because it's hidden by default, I like control button more and file menu is bad structured and scanty.

OK, then can you add the new Show Original item to the File menu in this patch? Let's not make things even worse in patches where we add something to the context menu.

Maybe we should even add it to the Control menu too, as it's contextual and won't appear there all the time, so it won't amount to much clutter.

No, I don't want to add this action right now because I don't like the situation about actions. They are done completely non-systematically.

To be honest, I don't understand logic behind Control menu too. Why I can Select All files, but cannot create new files or copy existing one via Control menu? Why do I need "Control menu -> Location Bar -> Replace Location" if I can just press on the Location Bar and achieve the same result? Why do we need "Editable Location" if it's the same as "Replace Location'?

In my opinion, we should do it like:

• Context menu. File actions on selected items. In other words, context menu is now almost perfect, except "New window" and "New tab".
• Control menu. Controlling the Dolphin (view, panels, etc.). We need to remove file operations ("Undo", "Select All", etc.).
• File menu. All operations. File menu = Context menu + Control menu.

In terms of programming, all actions creation should be moved into one place (File menu creation). Then we just copy existing actions into Context menu and Control menu.

I propose to start a new discussion, concerning this, with the Dolphin maintainers.

Dolphin's menubar situation isn't as dire as it seems, and I don't think it needs a big redesign. Window and tab-related items are in the File menu mostly for historical reasons, same as every other app with a file menu. Not ideal, but convention, traditional, familiarity, yadda yadda yadda.

The main problem is simply that Dolphin's File menu is missing most of the file-related items in the context menu. Fixing that is certainly outside the scope of this patch, but I don't think it's unreasonable to add Show Original to the File menu as a part of this patch since we're adding it to the context menu. If we don't add it to the File menu here as well, we're just making the existing problem worse.

In D10990#222356, @ngraham wrote:

Dolphin's menubar situation isn't as dire as it seems, and I don't think it needs a big redesign. Window and tab-related items are in the File menu mostly for historical reasons, same as every other app with a file menu. Not ideal, but convention, traditional, familiarity, yadda yadda yadda.
mostly for historical reasons

It's not excusable. On the other hand, I use File menu nor Control menu very little. If we do not agree, I will keep the situation as is.

The main problem is simply that Dolphin's File menu is missing most of the file-related items in the context menu. Fixing that is certainly outside the scope of this patch, but I don't think it's unreasonable to add Show Original to the File menu as a part of this patch since we're adding it to the context menu. If we don't add it to the File menu here as well, we're just making the existing problem worse.

OK. Where exactly do I insert Show Original?
Also as for now, File menu always show all actions, it disables actions that are not available (Copy when no files are selected, for example). I don't like the idea that File menu will be dynamical (I've never seen this in other programs), but showing "Show Original" all the time is not a good idea either.

In D10990#222446, @rominf wrote:

In D10990#222356, @ngraham wrote:

Dolphin's menubar situation isn't as dire as it seems, and I don't think it needs a big redesign. Window and tab-related items are in the File menu mostly for historical reasons, same as every other app with a file menu. Not ideal, but convention, traditional, familiarity, yadda yadda yadda.
mostly for historical reasons

It's not excusable.

It's also irrelevant: one issue per bug/patch. We can (and should, and will) clean up the menu later. For now, let's add Show Original into the file menu, where it belongs.

In D10990#222446, @rominf wrote:> OK. Where exactly do I insert Show Original?

Also as for now, File menu always show all actions, it disables actions that are not available (Copy when no files are selected, for example). I don't like the idea that File menu will be dynamical (I've never seen this in other programs), but showing "Show Original" all the time is not a good idea either.

There is a standard UI for this: you add the menu item to the right place, but make it disabled when it can't be used. See also: every other app with a menubar. For example, here's Gwenview:

Notice the disabled menu items.

Was there any code review on this?

Yes, I did the code review. Did I miss something obvious?

In D10990#225898, @ngraham wrote:

Yes, I did the code review. Did I miss something obvious?

I don't know, because I did not look at the code. But at least you could point out when "Accepting" what you did and what you didn't check, because you accepted things in the past were you clearly did not look at the code and only did QA (and I'm also not sure how familiar you are with Dolphin's code base, TBH).

An excellent point. I will remember that for the future and aspire to live up to your standards--and I hope everyone else does, too!

@elvisangelaccio Thanks for putting very clearly what I was trying to merely hint at.

@rominf @ngraham This repo is not your personal playground. Patches and reviews are appreciated, but for a happy community it is essential to show respect for every party involved. Elvis is working hard on lots of things, not reacting within 24h does not mean consent.

Thankfully, for future patches this should be easy to fix: Just have a bit more patience and wait for comments from a more experienced developer, you might even learn a thing or two ;)

In D10990#228470, @elvisangelaccio wrote:

Thanks, remember to use a descriptive commit message :)

@rominf If Elvis is hinting at something but you don't change anything, you should think twice before committing. This should have merely referred to the other commit with the same name and included the fact that here you are doing a follow-up commit to fix problems of the original commit.

Please respect that for future patches. Those requirements are not there to annoy you, but to keep the Git history usable for everyone working on Dolphin now and in the future.

In D10990#228540, @rkflx wrote:

In D10990#228470, @elvisangelaccio wrote:

Thanks, remember to use a descriptive commit message :)

@rominf If Elvis is hinting at something but you don't change anything, you should think twice before committing. This should have merely referred to the other commit with the same name and included the fact that here you are doing a follow-up commit to fix problems of the original commit.

Please respect that for future patches. Those requirements are not there to annoy you, but to keep the Git history usable for everyone working on Dolphin now and in the future.

I commited my changes and then landed them with:

git commit -am 'Rename "Show Original" -> "Show Target"'
arc land --revision D10990

As I understand arc simply ignored my commit message and used the message from the revision description. Had I change the revision description to "Rename "Show Original" -> "Show Target"" before landing?

Anyway I'm sorry.

UPD: Just found --preview option for arc land. I will use it in the future to avoid mistakes.

In D10990#228541, @rominf wrote:

git commit -am 'Rename "Show Original" -> "Show Target"'
arc land --revision D10990

arc will use whatever is set on Phabricator, the idea being that reviewers accept what they see and things are not changed behind their back. The technical reason is that arc land does arc amend to sync the latest Phab state (e.g. "Reviewed by") to your local checkout, overwriting your local changes. Please read our documentation again on how to change the summary.

In general it is easier to open a new Diff for new changes, and not work on an reopened Diff if it has not been reverted before.

In D10990#228541, @rominf wrote:

UPD: Just found --preview option for arc land. I will use it in the future to avoid mistakes.

I suspect arc amend is only called after --preview stops execution of arc land, so in that case you'd need --hold.

Edit: Also note that there is not a 1:1 relation between Phab's Diffs and commits, so it does not make even sense for arc to automatically use the latest commit message of the top-most patch. Phab operates on "ideas", which are squashed sets of commits.

I would also prefer a "in new tab" variant like you have when you right click a folder

In D10990#229068, @broulik wrote:

I would also prefer a "in new tab" variant like you have when you right click a folder

I think we're going to handle that more generally with https://bugs.kde.org/show_bug.cgi?id=183429

So I stumbled on something as a result of this patch. When navigating the Network(remote:/) path, right clicking an entry's "Show Original" action raises the following error.

Could not access "./network:/(I18N_ARGUMENT_MISSING)".

Message may vary per clicked item but the issue is still the same.

On a secondary note, what does "Show Original" on the remote:/ path even mean(yes I know they're symlinks)?
E.g.: right clicking on the bluetooth or "MTP Devices" entry and then "Show Original" comes up. Sounds a bit confusing.

In D10990#240003, @emateli wrote:

So I stumbled on something as a result of this patch. When navigating the Network(remote:/) path, right clicking an entry's "Show Original" action raises the following error.

Could not access "./network:/(I18N_ARGUMENT_MISSING)".

Message may vary per clicked item but the issue is still the same.

---

On a secondary note, what does "Show Original" on the remote:/ path even mean(yes I know they're symlinks)?
E.g.: right clicking on the bluetooth or "MTP Devices" entry and then "Show Original" comes up. Sounds a bit confusing.

Sounds like a bug to me; the menu item should only be displayed for actual symlinks, not those kind of locations. Feel free to file a bug or submit a patch!