1201456 - [Simulator] Be able to change TV mock data.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Simulator, defect)

Description

[Mantaroh Yoshinaga[:mantaroh]]

Current TV Manager API implementation, 
It can't change the mock data from UI.

The only way to change the mock data is to modify the JSON file.
It's not user friendly.

So we have better to create the setting panel in order to change mock data.
e.g. WebIDE / Mulet setting panel.. others.

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: [Image]TVDummySettingPanel

Hi Ryan,

I added the TV Simulator settings file to Gaia's profile.(see bug 180589/attachment #8667075 [details] [review])
The Web developer can edit this file.
(e.g. changing the TV Program Name or changing the video file)

However I think that this way of changing the file is not user friendly.
We may add the UI which can configurable TV Dummy data on WebIDE.

I would like to implement function of this, as several step.
1st Step) 
 Add the button like 'open tv dummy data' on the WebIDE's simulator setting panel[1].
  * This button displayed when selected the TV Simulator(see attachment).
  * I'm going to check the Simulator name(see bug 1201451).
  * It'd better to add the link of document which explained the settings format like MDN.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/webide/content/simulator.xhtml

2nd Step)
 Add the logic which save and load dummy settings file.
  * It's have no UI.
  * Load setting file when open the setting panel.

3rd Step)
 Add the config UI in order to change the settings.
  * Use the 2nd step logic.
  * UI is like the PasswordManager.(It will have the hierarchy tree)

What do you think about this plan?

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8670076 [details]
[Image]TVDummySettingPanel

Since Jan worked on simulator options panel, I'd like to get his feedback on this.

Comment 3

[Jan Keromnes [:janx]]

Comment on attachment 8670076 [details]
[Image]TVDummySettingPanel

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> Since Jan worked on simulator options panel, I'd like to get his feedback on
> this.

Thanks for forwarding this Ryan!

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> I added the TV Simulator settings file to Gaia's profile.(see bug
> 180589/attachment #8667075 [details] [review])
> The Web developer can edit this file.
> (e.g. changing the TV Program Name or changing the video file)
> 
> However I think that this way of changing the file is not user friendly.
> We may add the UI which can configurable TV Dummy data on WebIDE.

You're right Mantaroh, having to change the file is not user friendly, and even less so if we start packaging a TV simulator add-on (nobody should have to edit the files of an installed add-on manually).

> I would like to implement function of this, as several step.
> 1st Step) 
>  Add the button like 'open tv dummy data' on the WebIDE's simulator setting
> panel[1].
>   * This button displayed when selected the TV Simulator(see attachment).
>   * I'm going to check the Simulator name(see bug 1201451).
>   * It'd better to add the link of document which explained the settings
> format like MDN.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/webide/
> content/simulator.xhtml

In bug 1201451 comment 8, I suggest adding a Simulator type parameter to the Simulator configuration, and mention that it could help with type-specific features like your "Open TV Dummy Directory" button.

This button (and MDN link) would only show if the configured simulator is of type "television", so there is no need to check the Simulator name.

I am willing to write the patch for your 1st step once bug 1049704 has landed.

> 2nd Step)
>  Add the logic which save and load dummy settings file.
>   * It's have no UI.
>   * Load setting file when open the setting panel.

What would that logic look like? I'm not exactly sure what "save" and "load" mean for the dummy settings file. Can it just be a command-line argument to the simulator, e.g. ".../b2g-bin --dummy-data /folder/to/dummy/data"?

> 3rd Step)
>  Add the config UI in order to change the settings.
>   * Use the 2nd step logic.
>   * UI is like the PasswordManager.(It will have the hierarchy tree)

I'm not sure what this UI would look like. It's easy to add an "Open the Dummy Data Directory" button to let users modify that folder themselves, but it sounds a lot harder to create a complete configuration UI for the dummy settings (if you really need that, maybe this could be an add-on?).

I'm also not sure what PasswordManager or the hierarchy tree look like.

Comment 4

[Mantaroh Yoshinaga[:mantaroh]]

Thank you for your reply!

(In reply to Jan Keromnes [:janx] from comment #3)
> In bug 1201451 comment 8, I suggest adding a Simulator type parameter to the
> Simulator configuration, and mention that it could help with type-specific
> features like your "Open TV Dummy Directory" button.
> 
> This button (and MDN link) would only show if the configured simulator is of
> type "television", so there is no need to check the Simulator name.
> 
> I am willing to write the patch for your 1st step once bug 1049704 has
> landed.
OK.
If you don't mind, I would like to add the button and link of document to WebIDE when landed bug 1049704.
So could you review when I wrote patch?

> > 3rd Step)
> >  Add the config UI in order to change the settings.
> >   * Use the 2nd step logic.
> >   * UI is like the PasswordManager.(It will have the hierarchy tree)
> 
> I'm not sure what this UI would look like. It's easy to add an "Open the
> Dummy Data Directory" button to let users modify that folder themselves, but
> it sounds a lot harder to create a complete configuration UI for the dummy
> settings (if you really need that, maybe this could be an add-on?).
The idea of add-on is good.
In future, I would like to create those add-on in order to user can change settings file.

Comment 5

[Jan Keromnes [:janx]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #4)
> Thank you for your reply!

You're welcome :)

> If you don't mind, I would like to add the button and link of document to
> WebIDE when landed bug 1049704.
> So could you review when I wrote patch?

Sure! I can do that.

> > > 3rd Step)
> > >  Add the config UI in order to change the settings.
> > >   * Use the 2nd step logic.
> > >   * UI is like the PasswordManager.(It will have the hierarchy tree)
> > 
> > I'm not sure what this UI would look like. It's easy to add an "Open the
> > Dummy Data Directory" button to let users modify that folder themselves, but
> > it sounds a lot harder to create a complete configuration UI for the dummy
> > settings (if you really need that, maybe this could be an add-on?).
> The idea of add-on is good.
> In future, I would like to create those add-on in order to user can change
> settings file.

OK, good idea. This might require some changes in WebIDE (not sure yet), but we can work together on this.

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: WIP: Add button of opening tv dummy directory.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16ae24746678

Comment 8

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add button of opening tv dummy directory.

Hi janx.

I added the opening button when TV Simulator settings, and test codes. If focused simulator is phone, this settings menu is hidden. 

Could you confirm the patch?

Thanks.

Comment 9

[Jan Keromnes [:janx]]

Comment on attachment 8695767 [details] [diff] [review]
Add button of opening tv dummy directory.

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

I tried your patch and it works fine! The test as well, thank you for adding this feature.

I have a few remarks about the code, please either apply the changes I suggest or refuse them by replying to them.

::: devtools/client/locales/en-US/webide.dtd
@@ +214,5 @@
>  <!ENTITY simulator_hardware "Hardware">
>  <!ENTITY simulator_device "Device">
>  <!ENTITY simulator_screenSize "Screen">
>  <!ENTITY simulator_pixelRatio "Pixel Ratio">
> +<!ENTITY simulator_tv_data "TV Configuration">

Nit: I feel that "TV Simulation" would make more sense than just "Configuration" here (maybe we'll have other TV simulation features here?)

@@ +216,5 @@
>  <!ENTITY simulator_screenSize "Screen">
>  <!ENTITY simulator_pixelRatio "Pixel Ratio">
> +<!ENTITY simulator_tv_data "TV Configuration">
> +<!ENTITY simulator_tv_data_open "Config Data">
> +<!ENTITY simulator_tv_data_open_button "View config directory">

Nit: Maybe "Open Configuration Directory…" would be cleaner here? (s/View/Open/, capital letters, full words and the unicode "…" to indicate that we'll open a new window).

::: devtools/client/webide/content/simulator.js
@@ +118,5 @@
>        this.updateProfileSelector();
>        this.updateDeviceSelector();
>        this.updateDeviceFields();
> +
> +      // Change visibility of 'TV Simulator Menu'

Nit: Please add a '.' at the end of this comment.

@@ +126,5 @@
>      });
>    },
>  
> +  // Open the directory of TV Simulator config.
> +  showTVConfigDirectory() {

Nit: Please add a space before the '()'.

::: devtools/client/webide/content/simulator.xhtml
@@ +75,5 @@
>            </label>
>          </li>
>        </ul>
>  
> +      <!-- This menu shown when simulator type is television-->

Nit: Please add 'is' between 'menu' and 'shown'.

::: devtools/client/webide/test/test_simulators.html
@@ +297,5 @@
>            ok(params.args[sid + 1].includes(device.width + "x" + device.height), "Simulator screen resolution looks right");
>  
> +          // Test Simulator Menu
> +          is(doc.querySelector("#tv_simulator_menu").style.visibility, "hidden", "OpenTVDummyDirectory Button is not hidden\n");
> +      

Nit: Please remove these trailing spaces.

Comment 10

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add button of opening tv dummy directory.

Thanks janx!

I modified and rebased.

(In reply to Jan Keromnes [:janx] from comment #9)
> @@ +126,5 @@
> >      });
> >    },
> >  
> > +  // Open the directory of TV Simulator config.
> > +  showTVConfigDirectory() {
> 
> Nit: Please add a space before the '()'.
Did you mean that modify to |showTVConfigDirectory ()|? I think that it is not necessary.

Thanks.

Comment 11

[Jan Keromnes [:janx]]

Comment on attachment 8697031 [details] [diff] [review]
Add button of opening tv dummy directory.

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

Thank you for this new patch!

You forgot to address 2 nits, please fix them and re-upload the patch.

No need to ask for review again on the new patch.

::: devtools/client/locales/en-US/webide.dtd
@@ +214,5 @@
>  <!ENTITY simulator_hardware "Hardware">
>  <!ENTITY simulator_device "Device">
>  <!ENTITY simulator_screenSize "Screen">
>  <!ENTITY simulator_pixelRatio "Pixel Ratio">
> +<!ENTITY simulator_tv_data "Configuration">

Nit: "TV Simulation".

@@ +216,5 @@
>  <!ENTITY simulator_screenSize "Screen">
>  <!ENTITY simulator_pixelRatio "Pixel Ratio">
> +<!ENTITY simulator_tv_data "Configuration">
> +<!ENTITY simulator_tv_data_open "Config Data">
> +<!ENTITY simulator_tv_data_open_button "Open config directory">

Nit: "Open Configuration Directory…".

Comment 12

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Add button of opening tv dummy directory.

Thanks janx.

I address it, and rebased.
Carrying forward 'r+' from janx.

Comment 13

[Mantaroh Yoshinaga[:mantaroh]]

https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/9c2bc850c9c9

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9c2bc850c9c9

Comment 16

[Francesco Lodolo [:flod]]

Can we please fix the string to use … and not ... (note also that's what comment 11 requested)? That's the standard across all Mozilla products.

Comment 17

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Fix button caption of WebIDE.

Thanks flod,

I'm sorry for late reply.
I replaced the char ... to … . Could you confirm this char is correct?

Comment 18

[Francesco Lodolo [:flod]]

Comment on attachment 8703907 [details] [diff] [review]
Fix button caption of WebIDE.

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

It's correct, thanks.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c4168d010550

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c4168d010550