Don't show top "Open With" app for folders; only for files
ClosedPublic

Authored by ngraham on Apr 14 2018, 7:45 PM.

Details

Summary

This patch returns the Open With items for folders to the way it was before D11569: with all entries shown in a sub-menu. This should make the context menu more relevant since most of the time when you right click on a folder, it's not to open it in some app.

The top app is still displayed inline for the app context menu--just not for folders.

Test Plan

Top app Open With entries...

  • Do not appear for local and remote folders and symlinks to folders
  • Appear for local and remote files and symlinks to files

Folder:

File:

Note: screenshots reflect the Open With positioning from D11884: Move "Open" actions to the top of the context menu for files

Diff Detail

Repository
R241 KIO
Branch
no-open-with-for-folders (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
ngraham created this revision.Apr 14 2018, 7:45 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 14 2018, 7:45 PM
ngraham requested review of this revision.Apr 14 2018, 7:45 PM
ngraham edited the test plan for this revision. (Show Details)Apr 14 2018, 7:46 PM

I find "Open with..." for folders useful. "Open with VLC" is of course nonsense.

I find "Open with..." for folders useful.

What apps do you typically open folders in? What's the use case?

"Open with VLC" is of course nonsense.

Huh? Not sure I understand.

michaelh added a comment.EditedApr 14 2018, 8:02 PM

I find "Open with..." for folders useful.

What apps do you typically open folders in? What's the use case?

  • Watching Image galleries
  • Watching video series
  • Filelight/KDirstat
  • KdeSvn

"Open with VLC" is of course nonsense.

Huh? Not sure I understand.

The default app for folders is dolphin. So the entry above "Open with..." should be dropped.

So you're proposing that folders show the Open With... entry, but omit the recently added additional entry that shows the first app? Sounds reasonable enough to me. Does anyone else feel otherwise?

Zren added a subscriber: Zren.Apr 14 2018, 8:21 PM
  • Open with "Sublime Text" / "Atom"
  • Open with "K4DirStat"
  • Open with "Clementine"

Comix, Gwenview, VLC, $MediaPlayer will usually "autoplay" the next file in a folder, so this won't really affect those apps.

Do we really need these apps to define a context menu "Action"? "Right click > Actions > Open with Sublime Text" seems weird.

You also lose the ability for a user to open with any application that supports it, rather than just the apps that define it in their share/applications/app.metadata.

  • Oddly, XFCE's Thunar doesn't show an "Open with" menu.
  • Does Gnome's Nautilus not show it either?
  • Does Window's File Explorer not show it?
  • Does OS X's file manager not show it?
Zren added a comment.Apr 14 2018, 8:35 PM

So you're proposing that folders show the Open With... entry, but omit the recently added additional entry that shows the first app?

Hmmm? Ah I missed D11569. So the proposal is we don't show the inline "Open with Gwenview" and only show the "Open With > Gwenview"? Like we did before D11569?

In D12206#246301, @Zren wrote:

So you're proposing that folders show the Open With... entry, but omit the recently added additional entry that shows the first app?

Hmmm? Ah I missed D11569. So the proposal is we don't show the inline "Open with Gwenview" and only show the "Open With > Gwenview"? Like we did before D11569?

Right, it would be like this:

...Which is what we had for directories before D11569.

So I guess the question is, do we go back to what we had before for directories, or remove the Open With feature from their context menu?

ngraham edited the summary of this revision. (Show Details)Apr 14 2018, 9:47 PM
ngraham edited the summary of this revision. (Show Details)
ngraham retitled this revision from Don't show "Open With" for folders, as was apparently originally intended to [RFC] Don't show "Open With" for folders, as was apparently originally intended.
ngraham updated this revision to Diff 32153.Apr 14 2018, 9:53 PM

Instead of removing the "Open With" entries entirely, only remove the inline "Open in <first app>" entry

ngraham retitled this revision from [RFC] Don't show "Open With" for folders, as was apparently originally intended to [RFC] Don't show top "Open With" app for folders; only for files.Apr 14 2018, 9:59 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)Apr 14 2018, 10:01 PM
rkflx edited reviewers, added: elvisangelaccio; removed: rkflx.EditedApr 14 2018, 11:18 PM
rkflx added a subscriber: elvisangelaccio.

@ngraham The way I understood @elvisangelaccio was that the possibility to open a folder with another application should stay, but be featured less prominently (i.e. like before). My ping was mainly to get the conversation going again, because it looked like Elvis' concern was being ignored.

I would not agree with completely removing that functionality, because it is actually useful sometimes, e.g. for KDirStat/Filelight etc., and has been a feature even in Konqueror since more than a decade.

+1 for only showing the Open With menu, without an additional top app. (But make sure to consolidate the number of separators…)

@elvisangelaccio Please comment, so we'll know what you had in mind ;)


According to a comment in the code, Open With was never supposed to show up for folders in the first place, but a bug prevented this from working: the value of firstItem.url().isLocalFile() was also checked, but this always returns true for local directories, not just local files.

Are you sure? The API docs say that "A URL is a local file path if the scheme is "file".", i.e. it also applies to directories. The way I interpret not very useful, especially for remote folders, the way the condition is written and how it works in reality, is that only for remote folders the entry should be hidden.

rkflx added a subscriber: rkflx.Apr 14 2018, 11:29 PM
ngraham updated this revision to Diff 32161.Apr 15 2018, 3:51 AM

Restore original logic, minus the unnecessary extra separator

Are you sure? The API docs say that "A URL is a local file path if the scheme is "file".", i.e. it also applies to directories. The way I interpret not very useful, especially for remote folders, the way the condition is written and how it works in reality, is that only for remote folders the entry should be hidden.

Ah, thanks for the clarification, that makes sense (but yes, let's wait for @elvisangelaccio to confirm).

ngraham updated this revision to Diff 32162.Apr 15 2018, 3:53 AM

Restore original inconsistent whitespace

ngraham retitled this revision from [RFC] Don't show top "Open With" app for folders; only for files to Don't show top "Open With" app for folders; only for files.Apr 15 2018, 4:03 AM
ngraham edited the summary of this revision. (Show Details)
ngraham updated this revision to Diff 32164.Apr 15 2018, 4:09 AM

Simplify code

Yes, what I meant was to show only the "Open with" entry for folders.

Ok cool. That's what this patch does.

src/widgets/kfileitemactions.cpp
611

Why are we removing this separator?

Henrik suggested it:

+1 for only showing the Open With menu, without an additional top app. (But make sure to consolidate the number of separators…)

But I can do that in another patch.

ngraham updated this revision to Diff 32196.Apr 15 2018, 4:41 PM

Return the top separator, and let's fix the excessive separator issue in another patch

elvisangelaccio added a comment.EditedApr 15 2018, 4:42 PM

Henrik suggested it:

+1 for only showing the Open With menu, without an additional top app. (But make sure to consolidate the number of separators…)

But I can do that in another patch.

Ah right. Then we should do it in this patch, it's part of the same (atomic) change. (and we should mention in the commit message...)

ngraham updated this revision to Diff 32197.Apr 15 2018, 4:43 PM
ngraham marked an inline comment as done.

Revert unrelated change for another issue that snuck in...

ngraham updated this revision to Diff 32198.Apr 15 2018, 4:44 PM

Remove the top separator again

As for D11569, make sure we don't break the konqueror test before pushing.

This revision is now accepted and ready to land.Apr 15 2018, 4:46 PM

Yep, I was planning on it.

ngraham updated this revision to Diff 32242.Apr 16 2018, 3:09 AM

Revert unintantional whitespace change that Phabricator "helpfully" concealed

This revision was automatically updated to reflect the committed changes.