Fix for issue #1561 #1707

Merged
merged 7 commits into from Oct 3, 2012

Conversation

Projects
None yet
2 participants
@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

This comment has been minimized.

Show comment Hide comment
@njx

njx Sep 25, 2012

Member

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).

Member

njx commented Sep 25, 2012

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

This comment has been minimized.

Show comment Hide comment
@ckarande

ckarande Sep 25, 2012

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.

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

This comment has been minimized.

Show comment Hide comment
@njx

njx Sep 25, 2012

Member

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.)

Member

njx commented Sep 25, 2012

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

This comment has been minimized.

Show comment Hide comment
@ckarande

ckarande Sep 26, 2012

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?

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?

src/LiveDevelopment/main.js
+ $(LiveDevelopment).on("statusChange", function statusChange(event, status) {
+ //update the checkmark next to 'Live Preview' menu item
+ //add checkmark when status is STATUS_CONNECTING
+ if (status === LiveDevelopment.STATUS_CONNECTING) {

This comment has been minimized.

Show comment Hide comment
@njx

njx Sep 28, 2012

Member

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

@njx

njx Sep 28, 2012

Member

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

@njx

This comment has been minimized.

Show comment Hide comment
@njx

njx Sep 28, 2012

Member

Looks good--just had the one comment above.

Member

njx commented Sep 28, 2012

Looks good--just had the one comment above.

@ghost ghost assigned njx Sep 28, 2012

@njx

This comment has been minimized.

Show comment Hide comment
@njx

njx Sep 28, 2012

Member

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.)

Member

njx commented Sep 28, 2012

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.)

src/LiveDevelopment/main.js
+ function _setupGoLiveMenu() {
+ $(LiveDevelopment).on("statusChange", function statusChange(event, status) {
+ //update the checkmark next to 'Live Preview' menu item
+ //add checkmark when status is STATUS_CONNECTING

This comment has been minimized.

Show comment Hide comment
@njx

njx Sep 28, 2012

Member

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

@njx

njx Sep 28, 2012

Member

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

src/LiveDevelopment/main.js
+ CommandManager.get(Commands.FILE_LIVE_FILE_PREVIEW).setChecked(true);
+ } else if (status < LiveDevelopment.STATUS_CONNECTING) {
+ //remove checkmark if status is STATUS_INACTIVE or STATUS_ERROR
+ CommandManager.get(Commands.FILE_LIVE_FILE_PREVIEW).setChecked(false);

This comment has been minimized.

Show comment Hide comment
@njx

njx Sep 28, 2012

Member

If we only show the checkmark when the status is STATUS_ACTIVE, then you could just remove it in all other cases.

@njx

njx Sep 28, 2012

Member

If we only show the checkmark when the status is STATUS_ACTIVE, then you could just remove it in all other cases.

@njx

This comment has been minimized.

Show comment Hide comment
@njx

njx Sep 28, 2012

Member

Added a couple more notes

Member

njx commented Sep 28, 2012

Added a couple more notes

@ckarande

This comment has been minimized.

Show comment Hide comment
@ckarande

ckarande Sep 29, 2012

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

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

src/LiveDevelopment/main.js
+ function _setupGoLiveMenu() {
+ $(LiveDevelopment).on("statusChange", function statusChange(event, status) {
+ // Update the checkmark next to 'Live Preview' menu item
+ // Add checkmark when status is STATUS_ACTIVE

This comment has been minimized.

Show comment Hide comment
@njx

njx Oct 2, 2012

Member

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

njx Oct 2, 2012

Member

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

This comment has been minimized.

Show comment Hide comment
@njx

njx Oct 2, 2012

Member

One more little refinement suggestion.

Member

njx commented Oct 2, 2012

One more little refinement suggestion.

@ckarande

This comment has been minimized.

Show comment Hide comment
@ckarande

ckarande Oct 3, 2012

Ah..good point. Changed the code accordingly.

ckarande commented Oct 3, 2012

Ah..good point. Changed the code accordingly.

@njx

This comment has been minimized.

Show comment Hide comment
@njx

njx Oct 3, 2012

Member

Looks good, thanks. Merging.

Member

njx commented Oct 3, 2012

Looks good, thanks. Merging.

njx added a commit that referenced this pull request Oct 3, 2012

@njx njx merged commit 238f3ec into adobe:master Oct 3, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment