Include an emoji picker
ClosedPublic

Authored by apol on Oct 7 2019, 9:59 AM.

Details

Summary

Shows all emoji in categories, it will save the new emoji into the
clipboard.

Test Plan

Manual testing; run ibus-ui-emojier-plasma

Diff Detail

Repository
R119 Plasma Desktop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17435
Build 17453: arc lint + arc unit
apol created this revision.Oct 7 2019, 9:59 AM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 7 2019, 9:59 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
apol requested review of this revision.Oct 7 2019, 9:59 AM

Screenshot, please

apol edited the test plan for this revision. (Show Details)Oct 7 2019, 10:26 AM
apol updated this revision to Diff 67413.Oct 7 2019, 10:26 AM

Remove unused empty file

There's coding style issues, as it's new code you can hopefully just run clang-format over it.

applets/kimpanel/backend/ibus/emojier/emojier.cpp
66

leak of list?

81

there's a method in QAIM now called checkIndex() which can handle all this for you magically.

ngraham added a subscriber: ngraham.Oct 7 2019, 3:17 PM

Cool beans.

Can you make the drawer/sidebar more narrow like we do in Discover? The width is hardcoded in Kirigami to something that's just way too high for this application IMO.

Also an "all" category on top would be nice too. Many smartphone implementations also have a "recently/frequently used" category that's quite user-friendly.

apol marked 2 inline comments as done.Oct 7 2019, 10:58 PM
apol updated this revision to Diff 67484.Oct 8 2019, 10:33 AM

Include a recent category, other comments addressed

apol updated this revision to Diff 67485.Oct 8 2019, 10:39 AM

Icon for recent

From my POV, +1, though it would be worth outlining your longer term plans after this initial step.

Right now it doesn't really use the applet nor really do anything useful with IBUS, but I assume that's due to change.

applets/kimpanel/backend/ibus/emojier/ui/emojier.qml
30

needs a title, otherwise it has something technical in the screenshot

apol updated this revision to Diff 67486.Oct 8 2019, 10:45 AM

Add window title

apol marked an inline comment as done.Oct 8 2019, 10:52 AM

From my POV, +1, though it would be worth outlining your longer term plans after this initial step.

Right now it doesn't really use the applet nor really do anything useful with IBUS, but I assume that's due to change.

What I did was to implement an interface equivalent to /usr/lib/ibus/ibus-ui-emojier but with our tech and look & feel, so it can be summoned in a number of ways. I can consider integrating it tighter to the plasmoid, for now biggest asset ibus is offering is the emoji data and API (which is pretty good I'd say).

apol edited the test plan for this revision. (Show Details)Oct 8 2019, 10:53 AM
apol updated this revision to Diff 67511.Oct 8 2019, 3:32 PM

Forgot to add these files

How can I test this out? Do I need to have IBus set up?

mart added a subscriber: mart.Oct 11 2019, 12:23 PM

Cool beans.

Can you make the drawer/sidebar more narrow like we do in Discover? The width is hardcoded in Kirigami to something that's just way too high for this application IMO.

Also an "all" category on top would be nice too. Many smartphone implementations also have a "recently/frequently used" category that's quite user-friendly.

I think entrues should have icons and the sidebar be collapsible with just icons

In D24454#545325, @mart wrote:

I think entries should have icons and the sidebar be collapsible with just icons

Yes that would be nice too.

apol updated this revision to Diff 67711.Oct 11 2019, 3:09 PM

Address marco's comment

apol added a comment.Oct 11 2019, 3:10 PM

How can I test this out? Do I need to have IBus set up?

You just need to make sure it's installed, here we're only using it as a library. Then run the ibus-ui-emojier-plasma executable.

apol edited the test plan for this revision. (Show Details)Oct 11 2019, 3:11 PM
GB_2 added a subscriber: GB_2.Oct 11 2019, 3:15 PM

For "All" I think we should use an icon like view-list-icons, otherwise +1.

GB_2 added a reviewer: VDG.Oct 11 2019, 3:20 PM
GB_2 added a project: VDG.
GB_2 added a subscriber: VDG.
  1. For the first-run use case, show the All category instead of Recent, or any other time when "Recent" is empty
  2. How does it work? When I click on an emoji, the window disappears but nothing appears to happen. Its not in the clipboard either?
  3. Every time I move the cursor over an emoji, file:///usr/lib/qt/qml/QtQuick/Controls.2/org.kde.desktop/ToolTip.qml:48:5: Unable to assign [undefined] to bool is printed to the console
ngraham edited the test plan for this revision. (Show Details)Oct 11 2019, 3:42 PM

A few more:

  • List the categories in a more common order rather than alphabetical (i.e. first smilies, then nature, then food & drink, etc. Basically copy the mobile emoji pickers)
  • The window needs a proper icon, not the generic X11 icon
  • This might be in Kirigami, but the header text jumps around when I click on the X button to hide the drawer entirely:
apol updated this revision to Diff 67716.Oct 11 2019, 3:55 PM

Address Nate and GB comments

Oh and one more: ampersands aren't appearing in the category text in the sidebar:

Great, that's fixed the ampersand.

I still see this:

Every time I move the cursor over an emoji, file:///usr/lib/qt/qml/QtQuick/Controls.2/org.kde.desktop/ToolTip.qml:48:5: Unable to assign [undefined] to bool is printed to the console

...and all the observations in D24454#545454.

apol added a comment.Oct 11 2019, 4:24 PM

A few more:

  • List the categories in a more common order rather than alphabetical (i.e. first smilies, then nature, then food & drink, etc. Basically copy the mobile emoji pickers)

The list isn't hardcoded, I get it from the ibus dictionaries. By default there I get them in a random order.

  • The window needs a proper icon, not the generic X11 icon

Fixing

  • This might be in Kirigami, but the header text jumps around when I click on the X button to hide the drawer entirely:

I actually don't understand why I get an X at all

Oh one final thing I just thought of: it would be great if the search field could be in the main toolbar rather than in the sidebar, because this makes it inaccessible when the sidebar is collapsed, as it is by default. People might mistakenly assume that there's no search.

apol updated this revision to Diff 67724.Oct 11 2019, 4:45 PM

Make the main function more similar to discover's

Heh the sidebar ampersand fix appears to have had a side effect:

apol updated this revision to Diff 67725.Oct 11 2019, 4:54 PM

Move search field

apol updated this revision to Diff 67726.Oct 11 2019, 4:56 PM

Don't always show the search field

The new button in the toolbar should hide the search field if it's visible (i.e. it should be a toggle action), and the search field should be inside its own background or toolbar with appropriate padding, not just floating there in space.

But is there no way to integrate the search field into the main toolbar itself? There's plenty of room.

apol updated this revision to Diff 67730.Oct 11 2019, 5:21 PM

Fix on wayland (and x11?)

Don't ever close the process, leave it on background

hein added a subscriber: hein.Oct 12 2019, 10:34 AM

If I understand right, the emoji workflow this implements is via copy-paste, it can't directly insert into the text fields via the IME bus. Do you have a plan for that?

apol added a comment.Oct 12 2019, 2:47 PM
In D24454#545964, @hein wrote:

If I understand right, the emoji workflow this implements is via copy-paste, it can't directly insert into the text fields via the IME bus. Do you have a plan for that?

It has the exact same behaviour as ibus emoji. I did it like this because we first should reach the status quo before improving on it.

In D24454#545543, @apol wrote:

Fix on wayland (and x11?)

Don't ever close the process, leave it on background

Hmm, that seems kind of like a sledgehammer solution to the problem. Why isn't it managing to send the paste data to klipper before quitting?

Why isn't it managing to send the paste data to klipper before quitting?

How?

  1. We announce "I have some new data" to X.
  1. Then klipper sees that, then asks for the data
  1. Then (via X) we get that request and send the data.

It would be technically possible to tell when 2 has happened (nativeEventFilter for XCB_SELECTION_REQUEST) not sure of a feasible wayland equivalent. I can't see a hook for after stage 3.

Staying alive seems the most practical solution...but maybe we can make it stay alive for only 30s after the line emoticon copy event or something.

apol added a comment.Oct 13 2019, 11:02 PM

Why isn't it managing to send the paste data to klipper before quitting?

How?

  1. We announce "I have some new data" to X.
  2. Then klipper sees that, then asks for the data
  3. Then (via X) we get that request and send the data.

It would be technically possible to tell when 2 has happened (nativeEventFilter for XCB_SELECTION_REQUEST) not sure of a feasible wayland equivalent. I can't see a hook for after stage 3.

Staying alive seems the most practical solution...but maybe we can make it stay alive for only 30s after the line emoticon copy event or something.

Given that on Wayland we don't really have any klipper integration yet, I'd suggest thinking of alternatives later. In the end, this will also improve the startup time on subsequent runs.
It's exactly how krunner works, for example.

davidedmundson accepted this revision.Oct 14 2019, 11:49 AM
This revision is now accepted and ready to land.Oct 14 2019, 11:49 AM
This revision was automatically updated to reflect the committed changes.