After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 659275 - Add context menu to entries
Add context menu to entries
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on: 659270 659274
Blocks:
 
 
Reported: 2011-09-16 17:51 UTC by Florian Müllner
Modified: 2011-10-17 13:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
shell-entry: Create a small wrapper around St.Entry (2.98 KB, patch)
2011-09-16 17:51 UTC, Florian Müllner
none Details | Review
Port (some) uses of St.Entry to ShellEntry (18.28 KB, patch)
2011-09-16 17:51 UTC, Florian Müllner
none Details | Review
shell-entry: Add a context menu (6.63 KB, patch)
2011-09-16 17:51 UTC, Florian Müllner
none Details | Review
Port (some) uses of St.Entry to ShellEntry (18.37 KB, patch)
2011-09-19 17:02 UTC, Florian Müllner
needs-work Details | Review
shell-entry: Add a context menu (5.75 KB, patch)
2011-10-11 17:02 UTC, Florian Müllner
none Details | Review
shell-entry: Add API to support entry context menus (6.22 KB, patch)
2011-10-11 23:09 UTC, Florian Müllner
reviewed Details | Review
shell-entry: Add API to support entry context menus (6.79 KB, patch)
2011-10-13 00:14 UTC, Florian Müllner
committed Details | Review
Add context menus to some entries (5.61 KB, patch)
2011-10-13 00:19 UTC, Florian Müllner
committed Details | Review

Description Florian Müllner 2011-09-16 17:51:47 UTC
The main motivation for these patches is the removed functionality in bug 658948 - the designers agreed that using a switch to show the password is wrong and suggested a context menu instead; implement it, and add copy/paste actions while at it ...
Comment 1 Florian Müllner 2011-09-16 17:51:51 UTC
Created attachment 196761 [details] [review]
shell-entry: Create a small wrapper around St.Entry

For the time being, there are no real benefits apart from potentially
sharing style, but once functionality is added the wrapper will ensure
consistent behavior for all users.
Comment 2 Florian Müllner 2011-09-16 17:51:54 UTC
Created attachment 196762 [details] [review]
Port (some) uses of St.Entry to ShellEntry

Replace the use of St.Entry with ShellEntry, except for:

 - looking glass - it's a development tool, so consistency
                   does not matter that much there
 - notifications - the intention is to add a context menu to
                   ShellEntry; while this is useful in notifications
                   as well, it will require changes to the tray's
                   focus grab handling, so leave those entries out
                   for now
Comment 3 Florian Müllner 2011-09-16 17:51:59 UTC
Created attachment 196763 [details] [review]
shell-entry: Add a context menu

Allow to pop up a context menu on right-click/long-press. For now,
add "Copy"/"Paste" actions common to all entries, and an additional
"Show/Hide Text" toggle action for password entries.
Comment 4 Florian Müllner 2011-09-19 17:02:58 UTC
Created attachment 196974 [details] [review]
Port (some) uses of St.Entry to ShellEntry

Rebased to master
Comment 5 Florian Müllner 2011-10-11 17:02:06 UTC
Created attachment 198800 [details] [review]
shell-entry: Add a context menu

Use ClutterClickAction instead of handling low-level signals; if we want to, this should make it easier to change the API to something like:

  let entry = new St.Entry();
  ShellEntry.addContextMenu(entry, { foo: bar });
Comment 6 Florian Müllner 2011-10-11 23:09:16 UTC
Created attachment 198820 [details] [review]
shell-entry: Add API to support entry context menus

Add addContextMenu() to support context menus on right-click/long-press.
Depending on the parameters passed, the context menu only contains
"Copy"/"Paste" actions or an additional "Show/Hide Text" toggle action
for password entries.


This is an alternative approach making the entry wrapper private, so the impact on existing code is kept as small as possible.
Comment 7 Jasper St. Pierre (not reading bugmail) 2011-10-12 18:57:58 UTC
Review of attachment 198820 [details] [review]:

::: js/ui/shellEntry.js
@@ +22,3 @@
+
+        PopupMenu.PopupMenu.prototype._init.call(this, entry, 0, St.Side.TOP);
+        this.setSourceAlignment(0.05);

A grep through my source shows that "setSourceAlignment" doesn't exist.

Also, why not the alignment parameter in the above _init?

@@ +67,3 @@
+    _updateCopyItem: function() {
+        let selection = this._entry.clutter_text.get_selection();
+        this._copyItem.setSensitive(selection && selection != '');

We should probably disallow copy/paste when it's a password entry, no? At least when "Show Password" isn't unchecked?

@@ +126,3 @@
+        clickAction.connect('clicked', Lang.bind(this, this._onClicked));
+        clickAction.connect('long-press', Lang.bind(this, this._onLongPress));
+        this.actor.add_action(clickAction);

Why both this.actor and this.actor.clutter_text?

@@ +140,3 @@
+    _onClicked: function(action, actor) {
+        if (this._menu.isOpen)
+            this._menu.close();

Won't menus have a grab, meaning that clicking anywhere while it's open will close it?

@@ +156,3 @@
+
+function addContextMenu(entry, params) {
+    return new _Entry(entry, params);

If your expected usage is:

    ShellEntry.addContextMenu(entry, { isPassword: true });

then what's the point of the returned thing?
Comment 8 Jasper St. Pierre (not reading bugmail) 2011-10-12 18:59:53 UTC
Review of attachment 196974 [details] [review]:

> - looking glass - it's a development tool, so consistency
>   does not matter that much there

I, personally would love copy/paste in a context menu there.

> will require changes to the tray's focus grab handling

More proof for bug #643687, I guess?

Anyway, this needs to be rebased to the current API anyway.
Comment 9 Florian Müllner 2011-10-12 22:14:51 UTC
(In reply to comment #7)
> A grep through my source shows that "setSourceAlignment" doesn't exist.

Yup, see bug 659274.


> Also, why not the alignment parameter in the above _init?

Mostly for convenience - the current behavior of pointing to the source's center is pretty much always what we want, so adding API for the one case where it's not feels nicer than requiring all users to update their constructors (alternatively we could move some of the parameters into a Param object with reasonable defaults, so future additions like this have less impact)



> @@ +67,3 @@
> +    _updateCopyItem: function() {
> +        let selection = this._entry.clutter_text.get_selection();
> +        this._copyItem.setSensitive(selection && selection != '');
> 
> We should probably disallow copy/paste when it's a password entry, no? At least
> when "Show Password" isn't unchecked?

That might make sense for copy (though I don't see how it's actively harmful - ctrl-c works as well), but paste? I'd consider it bad practice to keep "complicated" passwords like WEP keys in a text file and paste them into the entry, but I wouldn't go as far as "prohibiting" it in the interface (in quotes as we are not blocking ctrl-v either)


> @@ +126,3 @@
> +        this.actor.add_action(clickAction);
> 
> Why both this.actor and this.actor.clutter_text?

Yeah, this probably needs a comment; both actors are reactive, so in general the clutter_text will process the event. However if the CSS adds padding, adding a second action to the entry itself allows to popup the menu from the padding as well (which makes sense to me as entry and clutter_text appear as a single widget to the user, even though the padding is already excluded from the click area to activate the entry)


> @@ +140,3 @@
> +    _onClicked: function(action, actor) {
> +        if (this._menu.isOpen)
> +            this._menu.close();
> 
> Won't menus have a grab, meaning that clicking anywhere while it's open will
> close it?

Anywhere except for the menu's source actor.



> @@ +156,3 @@
> +
> +function addContextMenu(entry, params) {
> +    return new _Entry(entry, params);
> 
> If your expected usage is:
> 
>     ShellEntry.addContextMenu(entry, { isPassword: true });
> 
> then what's the point of the returned thing?

There's not much of a point, it's just a C-induced dislike of allocating an object and not allowing the caller to take a reference :-)
(we could get rid of the _Entry class altogether if we "attach" _menu and _menuManager directly to the entry - maybe we should just do that?)


(In reply to comment #8)
> Review of attachment 196974 [details] [review]:
> 
> > - looking glass - it's a development tool, so consistency
> >   does not matter that much there
> 
> I, personally would love copy/paste in a context menu there.

Sure, not a big deal adding it (slightly ot - I should probably remove the context menu from the login screen; the entry can be used both for username and password, and copy/paste just doesn't make sense there ...)


> > will require changes to the tray's focus grab handling
> 
> More proof for bug #643687, I guess?

Yes.


> Anyway, this needs to be rebased to the current API anyway.

Yes, if we want to go with the second approach.
Comment 10 Jasper St. Pierre (not reading bugmail) 2011-10-12 22:40:19 UTC
(In reply to comment #9)
> (In reply to comment #7)
> > A grep through my source shows that "setSourceAlignment" doesn't exist.
> 
> Yup, see bug 659274.
> 
> 
> > Also, why not the alignment parameter in the above _init?
> 
> Mostly for convenience - the current behavior of pointing to the source's
> center is pretty much always what we want, so adding API for the one case where
> it's not feels nicer than requiring all users to update their constructors
> (alternatively we could move some of the parameters into a Param object with
> reasonable defaults, so future additions like this have less impact)

Reviewed.

"It's what Windows does". WEP keys are a different case than what I was thinking of: real passwords. We should disable copy/paste/show password for the login screen and polkit dialog, IMO.

> > @@ +126,3 @@
> > +        this.actor.add_action(clickAction);
> > 
> > Why both this.actor and this.actor.clutter_text?
> 
> Yeah, this probably needs a comment; both actors are reactive, so in general
> the clutter_text will process the event. However if the CSS adds padding,
> adding a second action to the entry itself allows to popup the menu from the
> padding as well (which makes sense to me as entry and clutter_text appear as a
> single widget to the user, even though the padding is already excluded from the
> click area to activate the entry)

Won't bubbling take care of that?

> > @@ +140,3 @@
> > +    _onClicked: function(action, actor) {
> > +        if (this._menu.isOpen)
> > +            this._menu.close();
> > 
> > Won't menus have a grab, meaning that clicking anywhere while it's open will
> > close it?
> 
> Anywhere except for the menu's source actor.

Huh, that seems strange. Is it a bug?

> > @@ +156,3 @@
> > +
> > +function addContextMenu(entry, params) {
> > +    return new _Entry(entry, params);
> > 
> > If your expected usage is:
> > 
> >     ShellEntry.addContextMenu(entry, { isPassword: true });
> > 
> > then what's the point of the returned thing?
> 
> There's not much of a point, it's just a C-induced dislike of allocating an
> object and not allowing the caller to take a reference :-)
> (we could get rid of the _Entry class altogether if we "attach" _menu and
> _menuManager directly to the entry - maybe we should just do that?)

That's what I was thinking.
Comment 11 Jasper St. Pierre (not reading bugmail) 2011-10-12 22:58:20 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > A grep through my source shows that "setSourceAlignment" doesn't exist.
> > 
> > Yup, see bug 659274.
> > 
> > 
> > > Also, why not the alignment parameter in the above _init?
> > 
> > Mostly for convenience - the current behavior of pointing to the source's
> > center is pretty much always what we want, so adding API for the one case where
> > it's not feels nicer than requiring all users to update their constructors
> > (alternatively we could move some of the parameters into a Param object with
> > reasonable defaults, so future additions like this have less impact)

Now that I know what "alignment" is, could you put it where the user right-clicks instead of a constant 0.05? It probably means reworking the other code to take pixel values instead of a constant 0.5.
Comment 12 Florian Müllner 2011-10-13 00:14:29 UTC
Created attachment 198893 [details] [review]
shell-entry: Add API to support entry context menus

(In reply to comment #10)
> Won't bubbling take care of that?

No.

(In fact I noted now some oddities where ClutterText's event processing conflicts with the ClickAction, I'll deal with that tomorrow)


> > Anywhere except for the menu's source actor.
> 
> Huh, that seems strange. Is it a bug?

There's explicit code in PopupMenuManager, so I'd say it's deliberate :-)


> > > @@ +156,3 @@
> > > +
> > > +function addContextMenu(entry, params) {
> > > +    return new _Entry(entry, params);
> > > 
> > > If your expected usage is:
> > > 
> > >     ShellEntry.addContextMenu(entry, { isPassword: true });
> > > 
> > > then what's the point of the returned thing?
> > 
> > There's not much of a point, it's just a C-induced dislike of allocating an
> > object and not allowing the caller to take a reference :-)
> > (we could get rid of the _Entry class altogether if we "attach" _menu and
> > _menuManager directly to the entry - maybe we should just do that?)
> 
> That's what I was thinking.

OK, updated code accordingly.


(In reply to comment #11)
> Now that I know what "alignment" is, could you put it where the user
> right-clicks instead of a constant 0.05? It probably means reworking the other
> code to take pixel values instead of a constant 0.5.

Yes to the former, no to the latter - I think it would be confusing to have one "alignment" which is expressed in pixels; I could rename it to "sourceOffset", but that would put quite some burdon on existing code, so I prefer leaving it as alignment and calculate it from the right-click position.
Comment 13 Florian Müllner 2011-10-13 00:19:38 UTC
Created attachment 198894 [details] [review]
Add context menus to some entries

Use ShellEntry.addContextMenu() to add context menus to most
existing entries, with the exception of:

 - the login dialog - it may act be used to enter either the
                      username (e.g. no password entry) or the
                      password, and copy/paste does not make sense
                      (nowhere to copy from, nowhere to paste to)
 - notifications    - while adding a context menu is useful here as
                      well, it will require changes to the tray's
                      focus grab handling, so leave those entries
                      out for now
Comment 14 Jasper St. Pierre (not reading bugmail) 2011-10-13 00:52:20 UTC
Review of attachment 198893 [details] [review]:

Besides inactive copy/paste on real password entries, looks fine to me.

::: js/ui/shellEntry.js
@@ +42,3 @@
+        this._passwordItem = null;
+        if (params.isPassword) {
+            item = new PopupMenu.PopupMenuItem("");

single quotes for non-translated text.

@@ +167,3 @@
+    entry.connect('popup-menu', _onPopup);
+
+    entry._menu = new _EntryMenu(entry, params);

You should probably bail out if the menu already exists.
Comment 15 Jasper St. Pierre (not reading bugmail) 2011-10-13 00:53:37 UTC
Review of attachment 198894 [details] [review]:

You never seem to use styleClass. Is it that important to have it in the original patch?
Comment 16 Florian Müllner 2011-10-13 13:24:33 UTC
(In reply to comment #14)
> single quotes for non-translated text.

Fixed locally.


> @@ +167,3 @@
> +    entry.connect('popup-menu', _onPopup);
> +
> +    entry._menu = new _EntryMenu(entry, params);
> 
> You should probably bail out if the menu already exists.

Good point, changed locally.


(In reply to comment #15)
> You never seem to use styleClass. Is it that important to have it in the
> original patch?

It's even worse, the parameter is completely ignored in the shellEntry patch now ;-)

The original shellEntry patch (which wrapped St.Entry) used the parameter to allow overwriting the St.Entry's default style. I removed the left-over locally now.
Comment 17 Florian Müllner 2011-10-13 13:40:16 UTC
(In reply to comment #14)
> Besides inactive copy/paste on real password entries, looks fine to me.

It's a design question, but if we want to do this, we should go all the way:

  - have :can-copy/:can-paste properties on ClutterText which suppress the
    keyboard shortcuts accordingly

  - respect those properties in the context menu

This is obviously not possible for 3.2.x, but if it makes sense to disallow those actions, just hiding them looks like a rather poor approach to me ...
Comment 18 Jasper St. Pierre (not reading bugmail) 2011-10-17 12:47:57 UTC
Review of attachment 198893 [details] [review]:

::: js/ui/shellEntry.js
@@ +17,3 @@
+
+    _init: function(entry, params) {
+        params = Params.parse (params, { styleClass: null,

You never use this.
Comment 19 Florian Müllner 2011-10-17 13:07:42 UTC
(In reply to comment #18)
> Review of attachment 198893 [details] [review]:
> 
> ::: js/ui/shellEntry.js
> @@ +17,3 @@
> +
> +    _init: function(entry, params) {
> +        params = Params.parse (params, { styleClass: null,
> 
> You never use this.

See comment #16 - I've removed the parameter locally, I just thought it was unnecessary to attach another patch version.
Comment 20 Florian Müllner 2011-10-17 13:34:10 UTC
Attachment 198893 [details] pushed as 6257e64 - shell-entry: Add API to support entry context menus
Attachment 198894 [details] pushed as 9439da8 - Add context menus to some entries

Pushed after approval from both RT and i18n.