875679 - [STK] Metabug - Code refactoring

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia, defect)

Description

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Some STK raised bugs needs a code refactoring moving code from settings to system.

I suggest to leave in settings only navigation commands (STK_CMD_SET_UP_MENU and STK_CMD_SELECT_ITEM) all the other ones should be managed by system app.

With this architecture we avoid opening settings to start a phone call, send a short message, play tones, register events...

Comment 1

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

In this post I'll try to explain how we should do in order to agree the best solution and start working on it.

First of all I want to describe the most repeated issues:

    STK menu is showed after an UNSOLICITED STK command is received.
        For example, DISPLAY_TEXT or an alert which show the alert box and after that the settings app continues opened showing the STK main menu
        Worst is with commands like PLAY_TONE, SEND_SMS (without alert box), register events, ... which opens settings and nothing visual happens ... it's annoying
    After register some EVENTS they are lost if the user closes Settings app
        This is logic, after the STK command, settings is opened and the event is registered so each time the event happens (receiving a call, for example) this is notified by settings to the STK platform component
    Some events cann't be registered from settings
        User activity
        Lock screen enabled
        ...
        We need more privileges on settings to recover info about what's happening on other system areas

I can continue but I think this is clear enough.

How to solve these issues? my idea is to move most of the STK code to system leaving in settings only the navigation commands, so:

    STK_CMD_SET_UP_IDLE_MODE_TEXT
        Command to show a message into the idle screen and notification bar
        Now it's using notification helper to show it.
        Opening settings here is overkilling since only a notification helper call is needed
        Suggest: Move to System
    STK_CMD_REFRESH
        Command to clear the notification sent by IDLE MODE TEXT - Cann't be implemented without notification helper changes
        Anyway, move it to system
    STK_CMD_POLL_INTERVAL
    STK_CMD_POLL_OFF
        Not implemented yet
    STK_CMD_SET_UP_EVENT_LIST
        Register different kind of events. No visual iteraction with user is needed, so again it's not needed in settings
        Suggest: Move to System
    STK_CMD_SET_UP_CALL
        Start a phone call
        Again, not needed in settings
    STK_CMD_SEND_SS
    STK_CMD_SEND_USSD
    STK_CMD_SEND_SMS
    STK_CMD_SEND_DTMF
        All of them to sent a message
        Not needed in settings
    STK_CMD_LAUNCH_BROWSER
        Open the browser
        Now we open settings and then we open the browser ... annoying
    STK_CMD_PLAY_TONE
        Generate a sound
        Opening settings without user visual interaction
    STK_CMD_DISPLAY_TEXT
        Show a dialog to the user to show a message.
        Now we open settings
        Show the message on a fullscreen DIV
        The user close it and it's in the STK main menu ... this is annoying to the user
    STK_CMD_GET_INKEY
    STK_CMD_GET_INPUT
        Show a dialog to input text or to buttons asking a question. Same as DISPLAY_TEXT
    STK_CMD_SELECT_ITEM
        List of items inside a STK app.
        This is OK in settings
    STK_CMD_SET_UP_MENU
        List of STK apps available into the SIM Card
        This is OK in settings
    STK_CMD_PROVIDE_LOCAL_INFO
    STK_CMD_TIMER_MANAGEMENT
        Not implemented yet

So as a resume I suggest:

    Leave in settings ONLY STK_CMD_SET_UP_MENU and STK_CMD_SELECT_ITEM
    The other commands implemented in System
    For the commands with a user iteraction (like GET_IN* or DISPLAY_TEXT) we can show them on a window like attention screen so it can be showed above any other app. This windows should have a lower z-index than attention screen to avoid affect answering calls.

This is a big refactoring. I can drive it if you agree but we need your point of view on this topics.

Comment 2

[Fabien Cazenave [:kaze]]

(In reply to Fernando R. Sela [:frsela] from comment #0)
> Some STK raised bugs needs a code refactoring moving code from settings to
> system.
> 
> I suggest to leave in settings only navigation commands (STK_CMD_SET_UP_MENU
> and STK_CMD_SELECT_ITEM) all the other ones should be managed by system app.
> 
> With this architecture we avoid opening settings to start a phone call, send
> a short message, play tones, register events...

Hey Fernando, your proposal makes sense but I’m quite reluctant to move code to the System app. Tim, would you be OK with that?

Comment 3

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

(In reply to Fabien Cazenave [:kaze] from comment #2)
> 
> Hey Fernando, your proposal makes sense but I’m quite reluctant to move code
> to the System app. Tim, would you be OK with that?

Kaze, thank you for loop me in on this. I would need more detail to make the call on this.

-- Is the STK handling code rather isolated in System app? Or it would need to probe properties/method of other scripts in the System app. We really cannot afford to make another half-broken ad-hoc FTU launcher and fix it's regression.
-- How would the code talk to Gecko? Would it block other script from accessing the same API (say, attach the callback through one and only onxxxx property)
-- How would the code talk to Settings app? Do we still rely on mozSettings for that?

I would tend to think the first one as a blocker, and others not so much, but something worth noticing when writing the code.

Comment 4

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #3)
> (In reply to Fabien Cazenave [:kaze] from comment #2)
> > 
> > Hey Fernando, your proposal makes sense but I’m quite reluctant to move code
> > to the System app. Tim, would you be OK with that?
> 
> Kaze, thank you for loop me in on this. I would need more detail to make
> the call on this.
> 
> -- Is the STK handling code rather isolated in System app? Or it would need
> to probe properties/method of other scripts in the System app. We really
> cannot afford to make another half-broken ad-hoc FTU launcher and fix it's
> regression.

it's an independent module, it will receive ICC STK commands to:

 * Play sounds
 * Register and notify events (many)
 * Display alerts and queries to the user that should be mandatory, so a popup or an attention screen will be used
 * Manage SMS, USSD, DTMF, Phone calls, ... initiated from STK app (only notify user and answer the user decission, the rest is made by the Modem)
 * Cache main menu (this is done now in system in icc_cache) and transfer the message to the settings app (select items and main menu only.)

But as a resume, is mainly independent of the rest of system app.


> -- How would the code talk to Gecko? Would it block other script from
> accessing the same API (say, attach the callback through one and only onxxxx
> property)

There is a component: window.navigator.mozIccManager only for this. It isn't blocker

> -- How would the code talk to Settings app? Do we still rely on mozSettings
> for that?
> 

Now it's using mozSettings to pass the message recovered by system (see icc_cache.js) but I would like a beautiful method... but this is what we have now.

> I would tend to think the first one as a blocker, and others not so much,
> but something worth noticing when writing the code.

If you need more info, I'm glad to help you.

Comment 5

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Simplified the management of debug traces for system and settings

This patch is intended to simplify the debug enabling/disabling for ICC development and also simplify the logcat captures for testing teams.

Comment 6

[Fabien Cazenave [:kaze]]

Comment on attachment 759685 [details]
Simplified the management of debug traces for system and settings

These debug options can make sense for Gecko features, but I don’t think we need those for Gaia features: it’s easy to set a DEBUG=true in the beginning of a JS file while testing — and I think it’s more flexible.

Comment 7

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Fabien Cazenave [:kaze] from comment #6)
> Comment on attachment 759685 [details]
> Simplified the management of debug traces for system and settings
> 
> These debug options can make sense for Gecko features, but I don’t think we
> need those for Gaia features: it’s easy to set a DEBUG=true in the beginning
> of a JS file while testing — and I think it’s more flexible.

Hi Kaze,

I understand your point of view and I agree that it's easy to change a flag before flash the handset but in the STK case is very difficult to reproduce the issues in development environment so it's needed to enable debug during testing process if some issue is detected but deactivate it for testing performance.

In the last months I had been writing the same paragraphs once and again, explaining testers how to enable DEBUG traces in both apps so I supposed this configuration flag will simplify the tester life avoiding them to reflash for each kind of test.

Other idea could be to fusion both DEBUG parameters so we'll only expose a "DEBUG ICC Enabled" flag which changes system & settings DEBUG flag.

WDYT?

Comment 8

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

Thanks for the response.

(In reply to Fernando R. Sela [:frsela] from comment #4)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #3)
> > (In reply to Fabien Cazenave [:kaze] from comment #2)
> > > 
> > > Hey Fernando, your proposal makes sense but I’m quite reluctant to move code
> > > to the System app. Tim, would you be OK with that?
> > 
> > Kaze, thank you for loop me in on this. I would need more detail to make
> > the call on this.
> > 
> > -- Is the STK handling code rather isolated in System app? Or it would need
> > to probe properties/method of other scripts in the System app. We really
> > cannot afford to make another half-broken ad-hoc FTU launcher and fix it's
> > regression.
> 
> it's an independent module, it will receive ICC STK commands to:
> 
>  * Play sounds
>  * Register and notify events (many)
>  * Display alerts and queries to the user that should be mandatory, so a
> popup or an attention screen will be used
>  * Manage SMS, USSD, DTMF, Phone calls, ... initiated from STK app (only
> notify user and answer the user decission, the rest is made by the Modem)
>  * Cache main menu (this is done now in system in icc_cache) and transfer
> the message to the settings app (select items and main menu only.)
> 
> But as a resume, is mainly independent of the rest of system app.

Hum, looks like the only thing that is worthy of worrying is the |displaying alerts|; From the SIM Lock dialog experience, the sim_lock.js eventually tangled with other system modules because we want the dialog to show up on some occasion and not to show up on another. I suspect this is the one place that the proposed stk_manager.js will be non-independent.

> > -- How would the code talk to Gecko? Would it block other script from
> > accessing the same API (say, attach the callback through one and only onxxxx
> > property)
> 
> There is a component: window.navigator.mozIccManager only for this. It isn't
> blocker

This is alright. Don't worry about it.

> > -- How would the code talk to Settings app? Do we still rely on mozSettings
> > for that?
> > 
> 
> Now it's using mozSettings to pass the message recovered by system (see
> icc_cache.js) but I would like a beautiful method... but this is what we
> have now.

OK

> > I would tend to think the first one as a blocker, and others not so much,
> > but something worth noticing when writing the code.
> 
> If you need more info, I'm glad to help you.

If you could identify the "displaying alert" part and make sure we won't do anything worser than sim_lock.js, than I don't think there is a reason not to put this in the System app. Thanks.

Comment 9

[Fabien Cazenave [:kaze]]

(In reply to Fernando R. Sela [:frsela] from comment #7)
> Other idea could be to fusion both DEBUG parameters so we'll only expose a
> "DEBUG ICC Enabled" flag which changes system & settings DEBUG flag.

Maybe we should have a more generic approach by enabling a DUMP function for all Gaia apps:
 • one pref to rule them all (e.g. `debug.gaia.enabled') — we could even make it true by default on non-production builds;
 • one shared library (`shared/js/debug/js') to expose a DUMP object that we’d use like a `console.log()' or a `dump()';
 • replace all current `console.log()' calls in Gaia by a DUMP() call.

In order to make this debug data easier to grep, this DUMP() function would automatically prepend the application name (e.g. relying on document.location) in the console.log message.

If you think it makes sense, let’s open a separate bug for that please.

Comment 10

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Fabien Cazenave [:kaze] from comment #9)
> (In reply to Fernando R. Sela [:frsela] from comment #7)
> > Other idea could be to fusion both DEBUG parameters so we'll only expose a
> > "DEBUG ICC Enabled" flag which changes system & settings DEBUG flag.
> 
> Maybe we should have a more generic approach by enabling a DUMP function for
> all Gaia apps:
>  • one pref to rule them all (e.g. `debug.gaia.enabled') — we could even
> make it true by default on non-production builds;
>  • one shared library (`shared/js/debug/js') to expose a DUMP object that
> we’d use like a `console.log()' or a `dump()';
>  • replace all current `console.log()' calls in Gaia by a DUMP() call.
> 
> In order to make this debug data easier to grep, this DUMP() function would
> automatically prepend the application name (e.g. relying on
> document.location) in the console.log message.
> 
> If you think it makes sense, let’s open a separate bug for that please.

Challenge accepted ;) - Bug 881672 :)

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Sorry it took me a while to reply the need info here. I'm really against moving the STK to the system app.

 STK menu is showed after an UNSOLICITED STK command is received.
        For example, DISPLAY_TEXT or an alert which show the alert box and after that the settings app continues opened showing the STK main menu

Nothing prevent you to close the app yourself.

        Worst is with commands like PLAY_TONE, SEND_SMS (without alert box), register events, ... which opens settings and nothing visual happens ... it's annoying

Nothing prevent you to not bring the app to the front and to close it.

    After register some EVENTS they are lost if the user closes Settings app
        This is logic, after the STK command, settings is opened and the event is registered so each time the event happens (receiving a call, for example) this is notified by settings to the STK platform component

I'm not sure to understand?

    Some events cann't be registered from settings
        User activity

You can use the IdleObserver for that.

        Lock screen enabled

There is a setting for it / Or you can use a setting for that.

        ...
        We need more privileges on settings to recover info about what's happening on other system areas


I really don't think the STK needs more privileges. In fact adding more surface to the system app sounds a very bad idea as it lives in the main process and ideally it should do as less things as possible.

Also for the notifications issues there has been some changes in the notifications on mozilla-central. Have you looked at them?

Tim, do you see any good reasons to move that to the system app? If there are no strong arguments except UX I feel like this should not happens and all the things that have landed should be backouted...

Comment 12

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #11)
> Tim, do you see any good reasons to move that to the system app? If there
> are no strong arguments except UX I feel like this should not happens and
> all the things that have landed should be backouted...

As long as things are relatively isolated, moving STK into System app doesn't make it any better or worse in any way. FWIW, the code I r+'d didn't comes with ugly dependency to other system modules (e.g. calling WindowManager.launch() or something).

I did overlook it's own UI and potential UX breakage though; but I believe that's something we could file bug and get it fixed, not something that tie to where the code laid.

Comment 13

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Hi Vivien,

The main reasons to move this code to system have been as follows:

* EVENTS

Some operator apps require to be notified by some events (like cell location changes). This kind of events are continously received so it's very inneficient to open and close settings each time a location change occurs (in a mobile phone this is continously ;)

Moreover, if you register events in settings app, each time it's closed the SIM card stop receiving these events and you should re-register it again each time settings is opened.

* UX

Other operator apps want to show messages to the user over any app (shall be attended by the user) now, it's managed openning settings but if you consider this use case is annoying: For example, the user is reading the e-mail and automatically settings is opened (with the corresponding animation) and the message is showed (1-2 secs). After that, settings is closed (as you suggest) but now the system is in the homescreen instead of the e-mail app ...

Under the new approach, the dialog is showed OVER e-mail and the user can see (thanks the alpha channel) his own e-mail in the background. The dialog shows "Operator message" so neither animations nor strange behaviour with this. When the user closes the alert, the e-mail app continue opened with any change (<1 sec and no annoying behaviour).

* Performance

Opening settings for each UNSOLICITED command is a heavy task ... only to show an alert or play a tone. STK app is a really simple logic (since it only shows the UI but without any logic).

So in system app is not very heavy load, only shows alerts/confirmation messages on demand and plays tones but without any complex logic.

With the previous approach (STK under settings) the STK message is managed by system and settings is opened to show the message or play the tone or whatever, with the new approach the system app continues attending the STK message (exactly the same) but the tone or message is showed directly by system avoiding openning an app such as settings.

* Conclusion

This movement is not a heavy load to system (only a little logic is managed) the UX is hugely improved and the CPU/memory comsuption is decreased a lot. I think it is an important step mainly for low-end devices.

An enhancement I've in my head is to lazy-loading each command the first time it's used so if the STK never plays a tone, the related code is never loaded which is good to system since users with a SIM without STK or without UNSOLICITED STK messages won't load the JS code.

Finally it's important to note that STK menu navigation continues living in settings app, and we're moving only the commands that can be attended in a light mode by system app.

As Tim told in comment #12, the dependency with other areas is minimal or non-existent and we can load it on demand (lazy loading) in a near future.

Hope it helps for clarifying the proposed changes.

Regards,
Fernando.

Comment 14

[BMO Automation]

Firefox OS is not being worked on