I disagree. 
org.zanata.webtrans.client.presenter.KeyShortcutPresenter#setContextActive is not called by KeyShortcutPresenter itself it's by all other Presenters so the responsibility remains unchanged. 

It complicates a lot of things. For KeyShortcutManager.handleIfMatchingShortcut, it needs to check two state in stead of one. And we have two KeyShortcutManager right now (one for normal key and one for attention key). If anyone wants to create another KeyShortcutManager, he has to keep this information in mind. I think this is even more bug prone. It's in GWT so no multi threading issue here. In fact it's the only responsibility for ShortcutContextAwareDialogBox, to set correct context. 

----- Original Message -----

> From: "David Mason" <notifications@github.com>
> To: "zanata/zanata-server" <zanata-server@noreply.github.com>
> Cc: "Patrick Huang" <pahuang@redhat.com>
> Sent: Thursday, September 5, 2013 2:39:07 PM
> Subject: Re: [zanata-server] rhbz996398 - short cut context should change
> when review comment pop up shows (#147)

> In
> zanata-war/src/main/java/org/zanata/webtrans/client/ui/ShortcutContextAwareDialogBox.java:
> > +   @Override
> > +   public void hide()
> > +   {
> > +      super.hide();
> > +      keyShortcutPresenter.setContextActive(ShortcutContext.Popup, false);
> > +      for (ShortcutContext shortcutContext : activeContext)
> > +      {
> > +         keyShortcutPresenter.setContextActive(shortcutContext, true);
> > +      }
> > +   }
> > +
> > +   @Override
> > +   public void show()
> > +   {
> > +      super.show();
> > +      activeContext = keyShortcutPresenter.getActiveContexts();

> Handling context switching is the responsibility of KeyShortcutPresenter -
> all the other places just tell that class that it should activate or
> deactivate a context that they own. This is different - this class is taking
> over some context handling responsibility that it shouldn't be touching, and
> it is easy to imagine bugs happening if other context
> acitvation/deactivation happens while a modal dialog is open. To avoid bugs,
> and to make sure people aren't forced to understand the internals of
> KeyShortcutPresenter when they are reading the code in this class, I really
> think it would be better to move this logic into KeyShortcutPresenter .

> —
> Reply to this email directly or view it on GitHub .