mozilla / r2d2b2g

Firefox OS Simulator is a test environment for Firefox OS. Use it to test your apps in a Firefox OS-like environment that looks and feels like a mobile phone.
https://addons.mozilla.org/en-US/firefox/addon/firefox-os-simulator/
Other
392 stars 139 forks source link

"firefoxos port" and "firefoxos start [port]" gcli commands #803

Closed rpl closed 10 years ago

rpl commented 11 years ago

simplify the detection of the debugger port dynamically allocated by the Simulator (should fix #802)

mykmelez commented 11 years ago

This seems useful (it'd come in handy for me as well), although I'd prefer to call it simply "port", and I think we'll need to do a bit more to satisfy the request in #802.

Also note pull #756, where @ochameau is adding an interface for launching the Simulator that lets the caller specify the port.

rpl commented 11 years ago

@mykmelez just updated as suggested:

harthur commented 11 years ago

firefoxos listen 6000? start seems too generic.

This command would help I think.

rpl commented 11 years ago

@harthur @mykmelez I've thought about it a bit to try to find a strategy to add a "firefoxos listen PORT" command as requested and keep its behaviour near the original "listen PORT" command, as much as possible:

- if simulator is not already running, it starts it on the specified port
- if simulator is already running and it's listening on a different
  port, it request the running simulator to open a secondary listener on
  the specified port
- if simulator is already running and it's listening on the specified port,
  it doesn't do nothing 

How it looks to you?

mykmelez commented 11 years ago

@harthur @mykmelez I've thought about it a bit to try to find a strategy to add a "firefoxos listen PORT" command as requested and keep its behaviour near the original "listen PORT" command, as much as possible:

  • if simulator is not already running, it starts it on the specified port
  • if simulator is already running and it's listening on a different port, it request the running simulator to open a secondary listener on the specified port
  • if simulator is already running and it's listening on the specified port, it doesn't do nothing

How it looks to you?

Overall, it looks better, but I'm concerned about the complexity of opening a secondary listener, both in the implementation and in the interface. And the semantics of firefoxos listen don't really match those of the listen command built into Firefox. In fact, they seem more like opposites, since listen enables remote debugging while optionally setting the port, whereas firefoxos listen sets the port while optionally enabling remote debugging.

So I don't think we should try to reuse listen. Rather, given @harthur's comment 23055932 in #802, I would return to the original command you implemented, firefoxos port, but persist the values across Firefox sessions, and provide a way to reset port selection to the default behavior, so the command works like this:

Then the user will have to (re)start the Simulator manually after changing the port, but I think that's ok, since the user will rarely change the port (if I understand @harthur correctly, she wants to "set it and forget it", i.e. set it once and then never change it again). And when they do, it's easy to use the firefoxos start command to (re)start the Simulator if needed.

rpl commented 11 years ago

@harthur @mykmelez I've updated this pull request following the last suggestions, with a couple of tweaks:

firefoxos port set NUMBER: set a persistent preferred simulator port

firefoxos port get:

firefoxos port reset: reset the persistent preferred simulator port

when simulator is started:

rpl commented 10 years ago

@mykmelez I've rebased this branch on the updated master to check it's working correctly once merged, and in the last commit I've addressed the signaled issues:

In a19e5ad I've commented out the "preferences" attribute in the package.json (renaming it to "!preferences"), and covering the "undefined simple-pref" value case.

How it looks?

mykmelez commented 10 years ago

It looks good!

Note that I just branched for the 5.0 release of the Simulator, after which I merged @ochameau's changes for the next major release of the Simulator into the master branch. That major release removes the Dashboard and most of the Simulator's other functionality, as the Dashboard has been integrated into Firefox as the App Manager, and other functionality (ADB, GCLI commands, etc.) should similarly be integrated into Firefox or become separate addons.

Thus I have landed this changeset on the release-5.0 branch, and we'll ship it with Simulator 5.0, which will be the last major release of the "classic" version of the Simulator.

Thanks for the nice feature!