Fix for issue #1561

[ckarande]

For #1561, added logic to add / remove checkmark next to 'Live Preview' menu item.

The fix is added not exactly at TODOs, and outside of the if/else block to address the case of attempting to open non-htm files in live preview. In that case, the LiveDevelopment.status stays in STATUS_INACTIVE in the else block, so had to explicitly check it before adding the checkmark.

Please let me know if any suggestions.

[njx]

Haven't tried this fix myself yet, but I think there might be a problem if the connection status changes due to something other than the menu command being executed. For example, if you open a live development connection, then go to the browser window and close it, the connection is closed, but this code won't get called (I think). The enablement logic should probably be in _setStatus() instead (which appears to be the bottleneck for status change notifications).

[ckarande]

You are right, the code doesn't get called if user goes to browser and closes the Live Preview window directly.

One option could be to add checkmark in LiveDevelopment module's _onConnect() and remove it in _onDisconnect(). This would not be a bottleneck for status change notifications. This would require importing CommandManager and Commands in LiveDevelopment module though.

[njx]

Ah, right, we should keep that dependency out of the LiveDevelopment module. The right thing to do is to set up a handler for the "statusChange" event to update the menu. You could add this in LiveDevelopment/main.js--take a look at _setupGoLiveButton() for how this works for the button in the toolbar. (You could create an analogous function, like _setupGoLiveMenu(), to attach the event handler, and move the registration of the command into that function. Or you could just go ahead and do it in the init() method below where we register the command.)

[ckarande]

Thanks for the suggestion. I have added _setupGoLiveMenu() in LiveDevelopment/main.js that updates the checkmark based on "statusChange" event. The menu item checkmark works as expected with this fix and changes are available in ckarande:issue1561 branch.

About moving the registration of the FILE_LIVE_FILE_PREVIEW command into _setupGoLiveMenu(), in existing code, this registration happens outside of the init(). Moving it inside _setupGoLiveMenu() would cause exception while opening bracktets. Am I missing anything?

[njx]

I think it would be better to only show the checkmark when we actually finish establishing the connection (STATUS_ACTIVE).

[njx]

Looks good--just had the one comment above.

[njx]

FYI, if you reference the actual bug number with a "#" before it in the description or a comment (like this: #1561), it will link to the actual bug, and put a back-link to the pull request from the bug. (I edited your description to add it.)

[njx]

Minor nit: we generally put one space after // and capitalize comments.

[njx]

Added a couple more notes

[ckarande]

Thanks @njx! I have made change as per your comments.

[njx]

Now that the following logic is simple, it would be cleaner to replace the whole if statement with:

CommandManager.get(Commands.FILE_LIVE_FILE_PREVIEW).setChecked(status === LiveDevelopment.STATUS_ACTIVE);

[njx]

One more little refinement suggestion.

[ckarande]

Ah..good point. Changed the code accordingly.

[njx]

Looks good, thanks. Merging.