1025923 - [Roku] Removing an entry from history will only take effect after restarting the Roku app

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

[Flaviu Cos]

Environment: 
Casting device: Roku 3;

Steps to reproduce:
1. Open the Firefox app on Roku;
2. Go to Recent History;
3. Press the menu (*) button on the remote;
4. Choose 'Remove from history' option from the list;
5. Chose 'Yes' to confirm;
6. Press 'Ok' button on the remote to go back the history page.

Expected result:
The history entry is successfully removed.

Actual result:
The video is still present in history, but it will be deleted after restarting the Roku app.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: remove-history v0.1

I realize this is mostly Greek to you, but the gist of the patch is:
1. roGridScreen data is backed in an array of arrays (m.history)
2. roGridScreen does not have a way to remove an individual item itself. You need to remove it from the backing array of arrays (m.history) and then reload the data into roGridScreen
3. You also need to update the visibility of the datasets. If the recent history dataset is empty, don't show it in the grid.

We already did #3 when initializing the grid, so I pulled the function out and I'm calling it from do places now.

I also found a bug where we allowed the user to attempt to remove the built-in default items, but those are hard coded. I added some code to figure out which history items are "removeable" and then adjust the popup menu with the right actions.

I tested this patch 3 or 4 ways and it seems to be working fine now.

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 8485456 [details] [diff] [review]
remove-history v0.1

Review of attachment 8485456 [details] [diff] [review]:
-----------------------------------------------------------------

::: app/source/recentHistory.brs
@@ +113,5 @@
> +
> +        ' Update the screen
> +        m.history[0] = getRecentHistory()
> +        m.screen.setContentList(0, m.history[0])
> +        m.setSelectedItem()

This method feels like it should take a parameter. Maybe selectNextItem() or something would be better.

@@ +140,4 @@
>                  HDPosterUrl: video.poster
>                  SDPosterUrl: video.poster
>                  videoURL: video.url
> +                removeable: true

s/removeable/removable/

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #2)

> > +        m.screen.setContentList(0, m.history[0])
> > +        m.setSelectedItem()
> 
> This method feels like it should take a parameter. Maybe selectNextItem() or
> something would be better.

Makes sense. I went with selectInitialItem.

> >                  videoURL: video.url
> > +                removeable: true
> 
> s/removeable/removable/

Oops, thanks

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

https://github.com/mozilla/firefox-roku/commit/a039dae129f76f11ce6c78bb2022d96d46900df5

Comment 5

[Flaviu Cos]

Verified as fixed in builds:
- Firefox Nightly for Roku v1.1
- Firefox beta for Roku v1.0
Device: Roku 2.