1100781 - Implement the recently used emojis feature

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

With the spec in bug 983043, we need a indexed DB to store the recently used emojis for the "Recent" page of the emoji layout.

Comment 1

[John Lu [:mnjul][Please NI or I'll miss]]

Please remember to check if PromiseStorage [1] can adapt to your use case :)

https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/shared/promise_storage.js

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Ray, if you are interested, please spend some time thinking about your proposal of implementing this feature :)

Previously there was a proposal I rejected in bug 1157558. We don't need to follow that.

Comment 3

[Ray Lin[:ralin]]

Yes, I'm interested in this. 

I'll take a look at previous proposal then try coming up with new one. Thanks.

Comment 4

[Ray Lin[:ralin]]

Tim, 

I have some quick thoughts about previous patch and new approaches:

1. We don't really need to create new dynamic page to offer recent keys. We can just create new section according to data save in Storage then unshift this section to current sections. This would happen in PanelView.

2. Recent used keys could be saved in a fixed size array. If clicked key is already in this array, move this key to first place, else just prepend the key.

3. As you mentioned in bug 1157558, models should only be accessible by ViewManager, but AFAIK, it seems no persistent Storage is used in ViewManager. We could implement it, but this function need to be more general instead of serving only recently used emojis.

4. I think that recent used emojis icon(might be a timer/clock icon) will need a place at the bottom row, so this should be adjusted and ask UX for suggestions(maybe after we made a workable prototype).

Thanks!!

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

As we discussed offline:

1. Implement a storage module and make it available under KeyboardApp, so the proposed PageView and the TargetHandler can access it with |app|.
2. KayoutRenderingManager would get the stored layout *asynchronously* to ensure ViewManager (and PageView) can always access the stored layout *synchronously*.
3. PageView render and refreshes from the stored layout every time it is being show.
4. The stored layout is updated from signals from TargetHandler every time there is a key input, but whether or not to update the data in the disk at the same time is up to research on performance.

Thank you for working on this!

Comment 6

[Ray Lin[:ralin]]

further offline discussion:

1. As Emoji use default IME in InputMethodManager, I think it is not proper to mix up with recent used logic in it. I'll try to implement this feature in standalone IME called emoji.
2. Persistent/In-memory array manipulation can be moved to emoji IME, and we could handle click event propagated from TargetHandlers as well.
3. InputManager started before Layout Manager, I need to ensure recent used keys are loaded asynchronously before rendering layout. 
4. For updating recent page, have to avoid PageView data being cached. That is, re-render recent used page every time it activates.

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] raylin:1100781-recently-used-emojis > mozilla-b2g:master

Comment 8

[Ray Lin[:ralin]]

Comment on attachment 8702516 [details] [review]
[gaia] raylin:1100781-recently-used-emojis > mozilla-b2g:master

I did this prototype which close to this scenario.

I think current approach is not appropriate as it let IME to modify page/layout. So, if we create a database for layout which has property like "requirePersistentData:true" and use certain naming convention such as "emojiLayoutData" as database name.

At LayoutManager._updateCurrentPage, we check the properties and inject data into layout before key normalization. Every page change can retrieve new data.
At IME, we save data to database every click to ensure consistency.

Would it be better than current solution? or we should leave IME and focus on initiation process.

Thanks!!!

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8702516 [details] [review]
[gaia] raylin:1100781-recently-used-emojis > mozilla-b2g:master

Look really good. Thanks!

I like the current design (that have the IMEngine manage the storage by itself) than having a shared layout storage managed by LayoutLoader or LayoutManager. I've left a few comments on the interface though.

Comment 10

[Ray Lin[:ralin]]

Comment on attachment 8702516 [details] [review]
[gaia] raylin:1100781-recently-used-emojis > mozilla-b2g:master

The content of this patch:

1. Add a IME for Emoji, which is in charge of recently used key model. 
2. Add a new page to emoji for recently used keys.
3. Change initialization process of IME in inputMethodManager. Now IME's init function will be resolved before return IME instance.
4. Add new updateByIMEngine property to layout page. Page's content can be modified by IME before rendering, and the page instance won't be cached by viewManager as well.


I think I will file another integration test bug for emoji.

Thanks!!!

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8702516 [details] [review]
[gaia] raylin:1100781-recently-used-emojis > mozilla-b2g:master

I would love to r+ this but I think I should better look at https://github.com/mozilla-b2g/gaia/pull/33690/files#r48663700 one more time just to be responsible too. Thanks for the patch.

Comment 12

[Ray Lin[:ralin]]

Comment on attachment 8702516 [details] [review]
[gaia] raylin:1100781-recently-used-emojis > mozilla-b2g:master

I've updated PR.

Thank for reminding me this issue, I learned an experience from it.

I add destroy function to pageView, which remove itself from its parentNode. The destroyed element will be garbage collected since it is unreachable from main document.

Thanks!

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8702516 [details] [review]
[gaia] raylin:1100781-recently-used-emojis > mozilla-b2g:master

Please add unit test to ensure ViewManager does call the `destroy` method when the condition is met.

LayoutPageView#destory might need test too.

Comment 14

[Ray Lin[:ralin]]

Added unit tests in ViewManager and LayoutPageView.
Thanks!!!

https://github.com/mozilla-b2g/gaia/commit/c7ba1a00a54973f50bdad989c7c194a043ef712b

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Keyboard demo updated: http://timdream.org/gaia-keyboard-demo/#emoji