Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trash and Removable Device Icons #677

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@philipl
Copy link
Contributor

commented Jan 19, 2018

This is a re-submission after the original pull request ( #619 ) was accidentally merged and then unmerged.

This set of changes adds Trash and Removable Device/Drive icons similar to what the Unity dock offers.

  • Trash icon that reflects whether trash is empty or not, and has an action to empty the trash
  • Icons for removable devices and mounted volumes with unmount and eject actions (as appropriate)
  • Preferences to control whether to show icons or not
  • Window tracking using either the original extended Ubuntu fileManager1 api added for Unity, or the new upstream Nautilus version (which works with Wayland)

dashtodock

To summarise the old discussion, this is mostly complete with a few issues that I wouldn't consider blockers:

  • No drag and drop for trash. I think the best thing to do here is let the gnome shell folks implement this for their desktop icons extension and then copy/use it. :-)
  • Trash icon cannot be pinned to the bottom of the bar as in Unity. This could be done by someone with more knowledge than me but I really struggled to understand the St layout code.
@AsciiWolf

This comment has been minimized.

Copy link

commented Feb 18, 2018

Nice! I hope that this will be merged soon to fix #173.

@corebots

This comment has been minimized.

Copy link

commented Feb 19, 2018

great! thanks!

@fredldotme

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

Regarding those 3 points you make, I have some questions to the first 2 points.
Writing here in the hope of getting the point across to GNOME upstream (especially the designers) sooner rather than too late for GNOME 4.

  • Might be interesting if some upstream GNOME developers have different plans, especially since extending "Wayland" (the set of protocols) with custom protocols seems to be a thing now.

  • Keeping trash/device separation intact seems to be a feasable goal AFAICT, though I could see Canonical/Ubuntu ditching their patches sooner rather than later. Which almost screams for a general protocol/API to be in place.

Is anybody else interested in getting those ideas back to upstream? I'd like to help in that case.

@fredldotme

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2018

Continuing:

I could see some difficulties (not getting the "full experience" on non-Ubuntu distros) with those options being turned on by default. I don't want to add insult to injury but would it make sense to keep those options turned off by default and be enabled by Ubuntu via gsettings overrides (in case they want to get the feature in quickly)?

@philipl philipl force-pushed the philipl:master branch from cc8232a to 827a336 Mar 21, 2018

@philipl philipl force-pushed the philipl:master branch from 827a336 to 5cdda59 Apr 10, 2018

@philipl philipl force-pushed the philipl:master branch 2 times, most recently from 454a1fd to 12fa1cb Apr 26, 2018

dash.js Outdated
this._removables = new Locations.Removables();

// Trash Icon
this._trash = new Locations.Trash();

This comment has been minimized.

Copy link
@franglais125

franglais125 May 13, 2018

Contributor

Would it be possible to create the _trash object only if enabled in the settings?

I actually disabled it a long time ago, and only today I realized why my system froze for a few seconds every time I deleted files: the call back to trash changes is still active!

@philipl philipl force-pushed the philipl:master branch 2 times, most recently from 5cdda59 to 26cacaf May 14, 2018

@philipl

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

I've updated it to not create the apps if the icons are hidden.

@philipl

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

@didrocks Do you have any interest in putting this functionality into the ubuntu dock?

@didrocks

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2018

@philipl: yes, once merged in Dash To Dock properly, I think we'll rebase on it and activate that feature right away. No window matching in Wayland (and I guess no Trash window matching either) would be great to see it fixed though.

@philipl

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2018

So, I have an idea now on how to implement window tracking that works with Wayland. There is no equivalent of the XID from X11, as Wayland doesn't allow windows to be enumerated. Fortunately for us, Nautilus is a GtkApplication and uses GtkApplicationWindows. These have unique per-window ids which are published as part of the GtkApplication DBus API. Mutter tracks these ids and you can get them from a Mutter window. So, if the nautilus window reporting uses the 'window object path' that is published over dbus, then we can match the windows up. I'll try and prototype this

@philipl

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2018

Good initial results. I need to do Wayland testing, but the fundamental approach works. Obviously it still requires a nautilus patch, but this will be less controversial than the xid based approach.

@philipl

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

Ok. I've implemented support for this new way of doing window mapping. For now, the patch series includes support for both the Xid and window-object-path mechanisms. There may be some virtue in keeping that as it means compatibility with shipped Ubuntu releases.

I've got an updated version of the nautilus patch here:

https://gitlab.gnome.org/philipl/nautilus/commit/e43d39dbffc93cd18e0064e390105ac90f96be43

@didrocks We should discuss the best way to approach upstreaming this patch, and you can, of course, take it into the Ubuntu build of nautilus.

@didrocks

This comment has been minimized.

Copy link
Collaborator

commented May 16, 2018

Sounds excellent! I would prefer that you MR against nautilus and try to get some feedback from Carlos and other upstreams! You can maybe refer this discussions telling it's going to help dash to dock and others extensions matching windows, like ubuntu-dock?

Ofc, need to ensure that it doesn't regress for GNOME Shell dash in particular when you only have a nautilus desktop icon without any Trash (that it still matches nautilus).

@philipl

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2018

@philipl philipl force-pushed the philipl:master branch from fa6e937 to 1f0f425 May 16, 2018

gnomesysadmins pushed a commit to GNOME/nautilus that referenced this pull request May 19, 2018

nautilus-application: Publish window->location mappings
This is a reworking of a long standing Ubuntu patch that publishes
the set of locations open in each Nautilus window. The motivation
for this change is that a desktop environment providing special
icons for things like removable devices and the trash can match
windows to those icons for highlighting purposes.

In the original incarnation, Unity provided these icons. In today's
world, I'm maintaining a set of patches for dash-to-dock/ubunut-dock
that provide these icons too.

The original implementation uses Xids to identify windows, but Xids
aren't a thing in Wayland so this mechanism is a dead end. Instead,
we can use the 'gtk application window object paths' which are
published over dbus by GtkApplications, including Nautilus.

Mutter already detects these, and makes them available on MetaWindows.

The original patch added the mapping property to the fileManager1
interface, and I have left that part as-is, but it's likely not to
be the right place to put it. fileManager1 is a generic interface
and a property that assumes a GTK behaviour doesn't seem right.

We could obviously add it to a new interface under org.gnome.Nautilus,
but this would be Nautilus specific - although there isn't a huge
scope for other file managers to implement this property, so perhaps
that's just fine.

dash-to-dock discussion is readable here:

micheleg/dash-to-dock#677

@philipl philipl force-pushed the philipl:master branch from 1f0f425 to 79c237f May 19, 2018

@philipl

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2018

@didrocks It's now merged into Nautilus. If you want to backport to 3.26, the patch is on my ubuntu-3-26 branch in gitlab.

@micheleg I'd love to get this merged; is there anything you want me to do? any specific comments?

@didrocks

This comment has been minimized.

Copy link
Collaborator

commented May 22, 2018

@philipl Excellent! :)

@philipl philipl force-pushed the philipl:master branch from 79c237f to 92bae18 Aug 29, 2018

@philipl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

@micheleg, do you have any feedback? As I said before, I'd like to get this merged, and if you can give me any guidance on that, I can work on it. Thanks!

@khurshid-alam

This comment has been minimized.

Copy link

commented Nov 20, 2018

@philipl
This actually broke the xid patch for unity.

Can we still keep the xid patch along with this one ?

@philipl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

@khurshid-alam, sorry, I don't know what you mean. The xid patch is for nautilus and nothing I do here can break it. Are you saying that window tracking doesn't work for you when using my dash-to-dock patches? I'm still running with the default ubuntu nautilus which only supports xid based tracking (vs the application id based tracking I got merged into upstream nautilus) and window tracking is working correctly for me here.

@khurshid-alam

This comment has been minimized.

Copy link

commented Nov 20, 2018

@philipl

Sorry I didn't explain myself well.

We plan to add that patch back into nautilus 3.30 in disco for unity dock (for unity7 desktop). So I was concerned if it could interfere with application based tracking. But now that I checked again it doesn't. Another option is to use id based tracking in unity dock as well so that we don't need yet another dbus property. I well let Trevinho (it's his patch) to look into this.

Thanks.

@3v1n0

This comment has been minimized.

Copy link
Collaborator

commented Nov 20, 2018

From a quick look at the code, it seems you've to give one method some priority... So try to use the XIDs mode only if the gtk one isn't available.

As nautilus in ubuntu 19.10 will probably provide both (or might be, while probably we'd try to avoid it)

@philipl

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

The existing code is aware of both mechanisms and prefers the GTK based one over XID, which seems correct. A nautilus with both mechanisms would work equally well using both but you want to prefer GTK because when running under Wayland, the XID info would be absent or useless.

I agree that you'd more likely want to drop the XID mechanism and update the unity dock instead to avoid ambiguity.

@philipl philipl force-pushed the philipl:master branch from 48a98e9 to 8027130 Nov 30, 2018

@philipl philipl force-pushed the philipl:master branch from 8027130 to 2d875c2 Dec 19, 2018

@AsciiWolf

This comment has been minimized.

Copy link

commented Apr 18, 2019

@jbicha Any chance this could be merged and added in Ubuntu (instead of the current desktop icons, just like on Unity)?

@3v1n0

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

@philipl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

I’ll rebase onto master in the next few days. I’ve been waiting for 19.04 and gnome 3.32 so I can do that.

philipl added some commits Oct 12, 2017

dash: Add Trash Icon
This change introduces a basic trash icon that can open and empty
the trash. I did not attempt to implement any sort of drag and
drop to put something in the trash.

The implementation follows the path of least resistence and creates
a DesktopAppInfo so that it can be represented by a regular AppIcon.

That has particular implications - most significantly being that
any actions offered by the icon have to be Execs of external programs,
rather than code.

In this case, we care about opening the trash and emptying it. We
can use the 'gio' utility to open the trash using the default file
manager, but emptying it is trickier.

You can use gio to empty the trash but there is no confirmation
dialog if you do this. Rather than implement such a dialog, I
decided to use the Nautilus EmptyTrash dbus call; this triggers
confirmation from Nautilus, but is obviously nautilus specific.

Finally, I did not attempt to pin the icon to the bottom of the
dash as Unity does. Given how elaborate the icon allocation logic
is, I couldn't bring myself to tackle it.
dash: Add Removable drive/device icons
Another Unity dock capability is showing icons for removable drives
and devices.

This change introduces such icons for these entities. As with
the Trash, we back these with DesktopAppInfo, and implement the
open/unmount/eject actions with the 'gio' utility.

In Unity, icons are shown for both mounted and unmounted entities,
and this behaviour is retained. Also retained is the practice of
not exposing an unmount operation for ejectable entities. We also
cannot show an unmounted icon if the entity has no activation root,
but I believe that most entities of this type are ejectable so it's
not a real problem. This limitation arises because the activation
root is how we know where to mount the entity.

Unlike the trash, the natural icon placement matches the behaviour
in Unity.
appIcons: Implement window tracking for removable devices and trash
In Unity, special logic is present that will map a Nautilus window
the removable device or trash icon in the dock. The key to making
this possible is a special dbus property that was patched into
Nautilus that allows us to find the window where a location is
open.

Once we have access to this Nautilus information, we can then jump
through a bunch of hoops to map the locations to MetaWindows and
then a little special-casing logic, link our dock icons to those
windows.

Now, the special icons will have a running process highlight and
window counts, and all the usual features of a running app.

We support both the Ubuntu-specific patched Xid based window
matching as well as the upstream GtkApplication based matching
that I added to Nautilus.

In a difference from Unity, I made no attempt to subtract the
special location windows from the Nautilus app; that would be a
bunch of work and the benefit is unclear.

When run with a Nautilus that supports neither method, we will
simply never see any linked windows, and the behaviour will be
the same as without this change.

@philipl philipl force-pushed the philipl:master branch from 2d875c2 to ca8c5c7 Apr 20, 2019

@philipl

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

I've now updated the patch set to work with latest master and squashed a bunch of the changes together.

fileManager1API: Deduplicate window with related locations in tabs
I was already deduplicating if a window had the identical location
open in tabs but if the locations were different but still under
the same removable device, we'd get two separate entries created
for that same window. This is now fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.