Created attachment 233636 [details] [review]
ibusCandidatePopup: Style updates according to design mockups

Make it look more like the mockups.

Created attachment 233637 [details] [review]
ibusCandidatePopup: Add pagination buttons

Makes switching candidates pages with pointer clicks possible.

Created attachment 233638 [details] [review]
ibusCandidatePopup: Make candidates reactive to pointer clicks

Allow for candidates to be selected by clicking on them.

For reference, the mockups are at the bottom of https://raw.github.com/gnome-design-team/gnome-mockups/master/input-sources/input-sources-summary.png

Created attachment 234062 [details] [review]
ibusCandidatePopup: Style updates according to design mockups

Make it look more like the mockups.

In order to do that we stop using PopupMenu and friends as it doesn't
really buy us anything and just makes it more cumbersome to add the
style classes we need.
--

Stop using PopupMenu.

Created attachment 234063 [details] [review]
ibusCandidatePopup: Add pagination buttons

--
Rebased

Created attachment 234064 [details] [review]
ibusCandidatePopup: Make candidates reactive to pointer clicks

--
Rebased

Created attachment 234065 [details] [review]
ibusCandidatePopup: Fix cursor positioning

--
This has always been broken but went unnoticed until now since none of
the commonly used engines currently depend on this method. The fix is
easy now anyway.

Gave this a quick test, some observations:

- Shouldn't the horizontal layout use left/right instead (or in addition to) up/down ?

- Looks like the arrows are initially insensitive, and I have to arrow to the second page before they become sensitive

(In reply to comment #9)
> - Shouldn't the horizontal layout use left/right instead (or in addition to)
> up/down ?

You mean left/right arrow keys right? That falls under the ibus engine domain. It's the engine, through the gtk+ ibus module, that's processing the key events and sending dbus signals to gnome-shell to update the UI. The engines I've tried all use left/right to move the text cursor inside the pre-edit string.

> - Looks like the arrows are initially insensitive, and I have to arrow to the
> second page before they become sensitive

Yes, I also noticed that with ibus-libpinyin. Again, it's how that particular engine works, it doesn't send all the candidates up front, only on demand. We could make the buttons always sensitive.

(In reply to comment #10)
...
> > - Looks like the arrows are initially insensitive, and I have to arrow to the
> > second page before they become sensitive
> 
> Yes, I also noticed that with ibus-libpinyin. Again, it's how that particular
> engine works, it doesn't send all the candidates up front, only on demand. We
> could make the buttons always sensitive.

The buttons definitely need to be sensitive if there are more pages of characters, and they buttons need to be insensitive or hidden (probably better hidden) if there is only one page of candidate characters - otherwise people will try to skip forward when there aren't any more characters to view.

It should also be noted that there has been some talk about making the forward/back buttons insensitive depending on whether you are on the first/last page. A conclusion hasn't been arrived at there though.

(In reply to comment #11)
> (In reply to comment #10)
> ...
> > > - Looks like the arrows are initially insensitive, and I have to arrow to the
> > > second page before they become sensitive
> > 
> > Yes, I also noticed that with ibus-libpinyin. Again, it's how that particular
> > engine works, it doesn't send all the candidates up front, only on demand. We
> > could make the buttons always sensitive.
> 
> The buttons definitely need to be sensitive if there are more pages of
> characters, and they buttons need to be insensitive or hidden (probably better
> hidden) if there is only one page of candidate characters - otherwise people
> will try to skip forward when there aren't any more characters to view.

As the author of an engine which frequently has less than one page of candidates, hiding the buttons in such a case seems the better way.

In addition to being less confusing, it has the added benefit to make the popup more "compact".

My engine often returns only 2 candidates. Adding the buttons to that means a significant increase of the popup size for nothing, as the buttons would be insensitive.

> It should also be noted that there has been some talk about making the
> forward/back buttons insensitive depending on whether you are on the first/last
> page. A conclusion hasn't been arrived at there though.

As mentioned on IRC some time ago, cycling through pages is a very desired feature of many users, for example in Hong Kong.

However, they (HK people) mostly use the space key to go to next page, and they expect that using the space key on the last page will cycle back to the first page.

So I don't think they'd be too much bothered by the insensitive arrows (which they would most likely not use anyway), as long as the cycling still works.

However, here is an alternative idea: maybe the "next" arrow could change, to become a "first" arrow when on the last page? 

Something similar to this (but vertically, in the mockup style) :
- next: https://raw.github.com/chalkers/famfamfam/master/resultset_next.png
- first: https://raw.github.com/chalkers/famfamfam/master/resultset_first.png

I've commonly seen this metaphor used, with the bar indicating the first or last of a set.

Could that work?

Design questions:

1. Should we hide the arrows for small numbers of candidates? How small is small?

2. How do we visually handle wrapping around, i.e. last->first and first->last page?

3. The engine tells us that N candidates go in a page. The last page might thus have [1:N] candidates. In that case the arrows, being at the end of the list, will move which makes the user have to hunt for them. I think we should move the arrows to somewhere else. This is a problem both in vertical and horizontal layouts.

*** Bug 689040 has been marked as a duplicate of this bug. ***

(In reply to comment #13)
> Design questions:
> 
> 1. Should we hide the arrows for small numbers of candidates? How small is
> small?

The arrows are for paging, aren't they? I would assume that we'd show them if there's more than one page.

> 2. How do we visually handle wrapping around, i.e. last->first and first->last
> page?

That depends on whether we show the number of pages. Didn't we agree that we were going to show them when we discussed this last?

> 3. The engine tells us that N candidates go in a page. The last page might thus
> have [1:N] candidates. In that case the arrows, being at the end of the list,
> will move which makes the user have to hunt for them. I think we should move
> the arrows to somewhere else. This is a problem both in vertical and horizontal
> layouts.

Seems like the popup should stay the same size, even if most of it is empty.

Review of attachment 234065 [details] [review]:

I'd love an extended commit message here explaining what was broken, how it could be triggered, and what this fix does.

::: js/ui/ibusCandidatePopup.js
@@ +161,3 @@
                                  this._cursor.set_size(w, h);
+                                 if (this._boxPointer.actor.visible)
+                                     this._boxPointer.setPosition(this._cursor, 0)

Missing semicolon.

Review of attachment 234062 [details] [review]:

::: data/theme/gnome-shell.css
@@ +2126,3 @@
 .candidate-index {
+    padding: 0 0.5em 0 0;
+    color: #cccccc

Missing semi.

::: js/ui/ibusCandidatePopup.js
@@ +42,2 @@
         else                    // VERTICAL || SYSTEM
+            this.actor.vertical = true;

in the constructor props:

vertical: this._orientation == IBus.Orientation.HORIZONTAL,

@@ +55,3 @@
 
+            this._candidateBoxes[i].first_child.text = ((indexes && indexes[i]) ? indexes[i] : '%x'.format(i + 1));
+            this._candidateBoxes[i].last_child.text = candidates[i];

eek, not the biggest fan of using first_child and last_child here. Maybe you should have a IBusCandidate class that has the two children, and they manage themselves?

@@ +111,2 @@
                                  else
+                                     this._preeditText.hide();

this._preeditText.visible = visible;

@@ +120,3 @@
                                                              attrs);
                              }));
         panelService.connect('show-preedit-text',

This seems like an oddly specific signal...

@@ +136,2 @@
                                  else
+                                     this._auxText.hide();

this._auxText.visible = visible;

@@ +156,2 @@
                                  else
+                                     this._candidateArea.actor.hide();

this._candidateArea.actor.visible = visible;

Review of attachment 234063 [details] [review]:

::: js/ui/ibusCandidatePopup.js
@@ +58,2 @@
             this.actor.vertical = false;
+            this.actor.remove_style_class_name('vertical');

Hm, maybe it's not the best advice to merge this into one thing then.

@@ +114,3 @@
         this._boxPointer.actor.visible = false;
         this._boxPointer.actor.style_class = 'candidate-popup-boxpointer';
+        Main.layoutManager.addChrome(this._boxPointer.actor);

Seems like an bugfix unrelated to pagination?

@@ +225,3 @@
                                                                    cursorPos % pageSize,
                                                                    lookupTable.is_cursor_visible());
+                                 this._candidateArea.setOrientation(lookupTable.get_orientation());

Any specific reason for the separate setOrientation call, since you only seem to use it here?

Review of attachment 234064 [details] [review]:

OK.

Created attachment 236168 [details] [review]
ibusCandidatePopup: Style updates according to design mockups

--
(In reply to comment #17)
> ::: js/ui/ibusCandidatePopup.js
> @@ +42,2 @@
>          else                    // VERTICAL || SYSTEM
> +            this.actor.vertical = true;
>
> in the constructor props:
>
> vertical: this._orientation == IBus.Orientation.HORIZONTAL,

This can change at anytime when IBus tells us to show another bunch of
candidates.

> @@ +55,3 @@
>
> +            this._candidateBoxes[i].first_child.text = ((indexes &&
> indexes[i]) ? indexes[i] : '%x'.format(i + 1));
> +            this._candidateBoxes[i].last_child.text = candidates[i];
>
> eek, not the biggest fan of using first_child and last_child here. Maybe you
> should have a IBusCandidate class that has the two children, and they manage
> themselves?

Not worth a class just for this really, but I agree that using
first/last_child here sucks. What about this patch?

Also fixed the other points.

Created attachment 236169 [details] [review]
ibusCandidatePopup: Add pagination buttons

--
* I'm hiding the buttons now in case we don't have multiple pages as
per Allan's comment 15.

(In reply to comment #18)
> @@ +114,3 @@
>          this._boxPointer.actor.visible = false;
>          this._boxPointer.actor.style_class = 'candidate-popup-boxpointer';
> +        Main.layoutManager.addChrome(this._boxPointer.actor);
>
> Seems like an bugfix unrelated to pagination?

Well, before this, we didn't need to receive input events in the
popup.

> @@ +225,3 @@
> +
> this._candidateArea.setOrientation(lookupTable.get_orientation());
>
> Any specific reason for the separate setOrientation call, since you only seem
> to use it here?

Just because setCandidates() had too many arguments and setting the
orientation didn't seem particularly related to setting candidates.
And since I was also adding another method to update the buttons
here...

Created attachment 236170 [details] [review]
ibusCandidatePopup: Make candidates reactive to pointer clicks

--
Rebased.

Created attachment 236171 [details] [review]
ibusCandidatePopup: Fix cursor positioning

After moving the dummy source actor, we still have to poke the
boxpointer so that it gets repositioned.

This has always been broken but went unnoticed until now since none of
the commonly used engines currently depend on this method. Thanks to
Mathieu Bridon for pointing it out.

Comment on attachment 236170 [details] [review]
ibusCandidatePopup: Make candidates reactive to pointer clicks

This was already a-c-n

Review of attachment 236171 [details] [review]:

OK.

Review of attachment 236168 [details] [review]:

Yes, I like this.

Review of attachment 236169 [details] [review]:

::: js/ui/ibusCandidatePopup.js
@@ +59,3 @@
+            this.actor.remove_style_class_name('vertical');
+            this.actor.add_style_class_name('horizontal');
+            this._previousButton.child = new St.Icon({ icon_name: 'go-previous-symbolic' });

Can you construct one St.Icon and just set the icon name?

@@ +95,3 @@
+            this._buttonBox.hide();
+            return;
+        } else {

Remove the else.

@@ +100,3 @@
+
+        if (page == 0 && !wrapsAround)
+            this._previousButton.reactive = false;

this._previousButton.reactive = wrapsAround || page > 0;
this._nextButton.reactive = wrapsAround || page < nPages - 1;

(In reply to comment #21)
> Just because setCandidates() had too many arguments and setting the
> orientation didn't seem particularly related to setting candidates.
> And since I was also adding another method to update the buttons
> here...

Might be better as a separate patch, then.

Created attachment 236184 [details] [review]
ibusCandidatePopup: Add pagination buttons

--
(In reply to comment #27)
> Review of attachment 236169 [details] [review]:
>
> ::: js/ui/ibusCandidatePopup.js
> @@ +59,3 @@
> +            this.actor.remove_style_class_name('vertical');
> +            this.actor.add_style_class_name('horizontal');
> +            this._previousButton.child = new St.Icon({ icon_name:
> 'go-previous-symbolic' });
>
> Can you construct one St.Icon and just set the icon name?

Oh, sure.

> @@ +100,3 @@
> +
> +        if (page == 0 && !wrapsAround)
> +            this._previousButton.reactive = false;
>
> this._previousButton.reactive = wrapsAround || page > 0;
> this._nextButton.reactive = wrapsAround || page < nPages - 1;

Neater indeed.

Created attachment 236185 [details] [review]
CandidateArea: make setOrientation() public

setCandidates() has too many arguments and setting the orientation
isn't particularly related with it. It might also be useful to switch
orientation without changing the candidates.

Review of attachment 236185 [details] [review]:

OK.

Review of attachment 236184 [details] [review]:

::: js/ui/ibusCandidatePopup.js
@@ +31,3 @@
+        this._buttonBox = new St.BoxLayout({ style_class: 'candidate-page-button-box' });
+
+        this._previousButton = new St.Button({ style_class: 'candidate-page-button' });

style_class: 'candidate-page-button candidate-page-previous'

@@ +33,3 @@
+        this._previousButton = new St.Button({ style_class: 'candidate-page-button' });
+        this._previousButton.add_style_class_name('candidate-page-button-previous');
+        this._previousButton.child = new St.Icon();

new St.Icon({ style_class: 'candidate-page-button-icon' });

@@ +36,3 @@
+        this._buttonBox.add(this._previousButton, { expand: true });
+
+        this._nextButton = new St.Button({ style_class: 'candidate-page-button' });

style_class: 'candidate-page-button candidate-page-next'

@@ +38,3 @@
+        this._nextButton = new St.Button({ style_class: 'candidate-page-button' });
+        this._nextButton.add_style_class_name('candidate-page-button-next');
+        this._nextButton.child = new St.Icon();

new St.Icon({ style_class: 'candidate-page-button-icon' });

@@ +74,3 @@
+        }
+        this._previousButton.child.style_class = 'candidate-page-button-icon';
+        this._nextButton.child.style_class = 'candidate-page-button-icon';

You shouldn't need to do this anymore.

Created attachment 236197 [details] [review]
ibusCandidatePopup: Add pagination buttons

--
Duh *slaps himself*

Review of attachment 236197 [details] [review]:

OK.

Attachment 236168 [details] pushed as 6cd1e38 - ibusCandidatePopup: Style updates according to design mockups
Attachment 236170 [details] pushed as c3ed936 - ibusCandidatePopup: Make candidates reactive to pointer clicks
Attachment 236171 [details] pushed as 7e24a69 - ibusCandidatePopup: Fix cursor positioning
Attachment 236185 [details] pushed as 235ec7c - CandidateArea: make setOrientation() public
Attachment 236197 [details] pushed as 4f8586d - ibusCandidatePopup: Add pagination buttons