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.

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

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.

Created attachment 196974 [details] [review]
Port (some) uses of St.Entry to ShellEntry

Rebased to master

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 });

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.

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?

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.

(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.

(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.

(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.

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.

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

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.

Review of attachment 198894 [details] [review]:

You never seem to use styleClass. Is it that important to have it in the original patch?

(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.

(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 ...

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.

(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.

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.