[gwt-contrib] Re: Add NativePreviewEvent to replace current EventPreview system (version 4)
This seems like the right model of behavior. I wonder if we could figure out a way to tweak the API exposing the behavior though when you get back from vacation? As a user, I think I might get confused by the interactions of setAlwaysAutoHide(), setAutoHidePartner, setAutoHide(true/false),isAutoHideEnabled(), and isAlwaysAutoHide(), so I'm hoping there is a way to represent the same information with fewer moving parts. Cheers, Emily On Mon, Dec 22, 2008 at 12:46 PM, wrote: > Reviewers: ecc, > > Description: > The latest version of this patch introduces > PopupPanel.setAlwaysAutoHide(). Unlike autoHide, alwaysAutoHide > indicates that the PopupPanel should autoHide even if it isn't at the > top of the stack. The only time it does not autoHide is if a modal > popup is open above it. > > We have also gone back to NativePreviewEvent.cancel()/consume(). > > Both the SuggestBox and MenuBar setAlwaysAutoHide to true. However, the > MenuBar overrides the event preview to handle all of its unique special > cases. > > Please review this at http://gwt-code-reviews.appspot.com/1801 > > Affected files: > > > reference/code-museum/src/com/google/gwt/museum/client/defaultmuseum/Issue1932.java > > > reference/code-museum/src/com/google/gwt/museum/client/defaultmuseum/VisualsForDialogBox.java > user/src/com/google/gwt/event/dom/client/DomEvent.java > user/src/com/google/gwt/event/dom/client/HasNativeEvent.java > user/src/com/google/gwt/event/shared/GwtEvent.java > user/src/com/google/gwt/event/shared/HandlerManager.java > user/src/com/google/gwt/user/client/DOM.java > user/src/com/google/gwt/user/client/Event.java > user/src/com/google/gwt/user/client/EventPreview.java > user/src/com/google/gwt/user/client/ui/DialogBox.java > user/src/com/google/gwt/user/client/ui/MenuBar.java > user/src/com/google/gwt/user/client/ui/PopupPanel.java > user/src/com/google/gwt/user/client/ui/SuggestBox.java > user/test/com/google/gwt/user/UISuite.java > user/test/com/google/gwt/user/client/EventTest.java > user/test/com/google/gwt/user/client/ui/PopupTest.java > > > -- "There are only 10 types of people in the world: Those who understand binary, and those who don't" --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Add NativePreviewEvent to replace current EventPreview system (version 3)
Actually, hold off on this review please. In my haste, forgot to update MenuBar and SuggestBox to take advantage of the new alwaysAutoHide boolean. http://gwt-code-reviews.appspot.com/814 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Add NativePreviewEvent to replace current EventPreview system
http://gwt-code-reviews.appspot.com/606/diff/1/7
File user/src/com/google/gwt/event/logical/shared/HasNativeEvent.java
(right):
http://gwt-code-reviews.appspot.com/606/diff/1/7#newcode32
Line 32: }
Should this interface be in gwt.event.dom.client instead?
http://gwt-code-reviews.appspot.com/606/diff/1/5
File user/src/com/google/gwt/event/shared/GwtEvent.java (right):
http://gwt-code-reviews.appspot.com/606/diff/1/5#newcode162
Line 162:
As we now have a protected kill, can we remove onRelease?
http://gwt-code-reviews.appspot.com/606/diff/1/9
File user/src/com/google/gwt/user/client/Event.java (right):
http://gwt-code-reviews.appspot.com/606/diff/1/9#newcode55
Line 55: public static class NativePreviewEvent extends
GwtEvent
Can we use the same pattern we use for the the other events? "Represents
a preview of a native event" perhaps?
http://gwt-code-reviews.appspot.com/606/diff/1/9#newcode76
Line 76: if (handlers != null) {
To allow the compiler to optimize this code out, I think you also need
to check if TYPE is not null.
http://gwt-code-reviews.appspot.com/606/diff/1/9#newcode81
Line 81: singleton.revive();
this can be in an else, as you do not need to revive if you just created
it.
http://gwt-code-reviews.appspot.com/606/diff/1/9#newcode86
Line 86: for (int i = numHandlers - 1; i >= 0; i--) {
Can we use the 1.5 style for construct here?
http://gwt-code-reviews.appspot.com/606/diff/1/9#newcode102
Line 102: static Type getType() {
This is normally public.
http://gwt-code-reviews.appspot.com/606/diff/1/9#newcode151
Line 151: *
We should note here that isCanceled will still return true.
http://gwt-code-reviews.appspot.com/606/diff/1/9#newcode196
Line 196:
On the other events, we decided not to expose the protected setter. Is
there a reason to here?
http://gwt-code-reviews.appspot.com/606/diff/1/9#newcode356
Line 356:
I think using ArrayList as the declaration is slightly more efficient.
http://gwt-code-reviews.appspot.com/606/diff/1/12
File user/src/com/google/gwt/user/client/ui/PopupPanel.java (right):
http://gwt-code-reviews.appspot.com/606/diff/1/12#newcode270
Line 270: private HandlerRegistration nativePreviewHandlerRegistration =
null;
we usually do not include nulls here to stop the compiler from
generating a useless initial statement.
http://gwt-code-reviews.appspot.com/606
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Add NativePreviewEvent to replace current EventPreview system
This review replaces: http://gwt-code-reviews.appspot.com/805/show http://gwt-code-reviews.appspot.com/606 --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Add NativePreviewEvent to replace current EventPreview system
Cool, looking forward to seeing the new patch! Per our conversation, onPreviewNativeEvent will be exposed, but will > only contain minimal code by default. The bulk of the code will be in a > new method previewNativeEvent() > Why expose onPreviewNativeEvent at all? -- "There are only 10 types of people in the world: Those who understand binary, and those who don't" --~--~-~--~~~---~--~~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~--~~~~--~~--~--~---
[gwt-contrib] Re: Add NativePreviewEvent to replace current EventPreview system
Emily -
I replied to all your comments. I'll send out a new patch for review
soon that addresses all of your issues.
- John
http://gwt-code-reviews.appspot.com/805/diff/1/5
File user/src/com/google/gwt/event/shared/GwtEvent.java (right):
http://gwt-code-reviews.appspot.com/805/diff/1/5#newcode139
Line 139: /**
On 2008/12/08 23:06:37, ecc wrote:
> Should this method be final? As we've regretted making onDetach not
final. Also,
> I think you meant rely
Per our conversation, it doesn't need to be final. Users may override
it to take action when an event is killed.
http://gwt-code-reviews.appspot.com/805/diff/1/9
File user/src/com/google/gwt/user/client/Event.java (right):
http://gwt-code-reviews.appspot.com/805/diff/1/9#newcode187
Line 187: protected void kill() {
On 2008/12/08 23:06:37, ecc wrote:
> can you document the method that requires access to kill here?
fire() has been moved into the Event, so this is no longer needed.
http://gwt-code-reviews.appspot.com/805/diff/1/9#newcode482
Line 482: NativePreviewEvent event = new
NativePreviewEvent(nativeEvent);
On 2008/12/08 23:06:37, ecc wrote:
> I think we might want to recycle the native event here for efficiency.
Agreed. Added to the latest version.
http://gwt-code-reviews.appspot.com/805/diff/1/11
File user/src/com/google/gwt/user/client/ui/DialogBox.java (left):
http://gwt-code-reviews.appspot.com/805/diff/1/11#oldcode231
Line 231:
On 2008/12/08 23:06:37, ecc wrote:
> As much as I'd really, really love to do this, I think backward
compatibility
> forces us to deprecate the method rather then removing it.
I'm just removing the override. onEventPreview is still deprecated in
the parent class.
http://gwt-code-reviews.appspot.com/805/diff/1/11
File user/src/com/google/gwt/user/client/ui/DialogBox.java (right):
http://gwt-code-reviews.appspot.com/805/diff/1/11#newcode392
Line 392:
On 2008/12/08 23:06:37, ecc wrote:
> do we want to expose this?
Per our conversation, onPreviewNativeEvent will be exposed, but will
only contain minimal code by default. The bulk of the code will be in a
new method previewNativeEvent()
http://gwt-code-reviews.appspot.com/805/diff/1/13
File user/src/com/google/gwt/user/client/ui/MenuBar.java (left):
http://gwt-code-reviews.appspot.com/805/diff/1/13#oldcode966
Line 966: public boolean onEventPreview(Event event) {
On 2008/12/08 23:06:37, ecc wrote:
> See comments from DialogBox.
We can remove the overide, and this is an anonymous inner class anyway.
http://gwt-code-reviews.appspot.com/805/diff/1/12
File user/src/com/google/gwt/user/client/ui/PopupPanel.java (right):
http://gwt-code-reviews.appspot.com/805/diff/1/12#newcode464
Line 464: */
On 2008/12/08 23:06:37, ecc wrote:
> Where is the onPreviewNativeEvent for this one?
Its protected, not public
http://gwt-code-reviews.appspot.com/805/diff/1/3
File user/test/com/google/gwt/user/client/EventTest.java (right):
http://gwt-code-reviews.appspot.com/805/diff/1/3#newcode26
Line 26: public class EventTest extends GWTTestCase {
On 2008/12/08 23:06:37, ecc wrote:
> Nice Test!
>
> I think the only thing you need to add is the interactions of code
that mixes
> the old and new styles.
> For example:
> Use adds a new style preview, then an old style preview. What order
should they
> be evaluated in?
Added in new version of patch.
http://gwt-code-reviews.appspot.com/805
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
[gwt-contrib] Re: Add NativePreviewEvent to replace current EventPreview system
This is looking very nice, I think the structure is great. We have a few
issues to work on which are mentioned in in-line comments.
http://gwt-code-reviews.appspot.com/805/diff/1/5
File user/src/com/google/gwt/event/shared/GwtEvent.java (right):
http://gwt-code-reviews.appspot.com/805/diff/1/5#newcode139
Line 139: /**
Should this method be final? As we've regretted making onDetach not
final. Also, I think you meant rely
http://gwt-code-reviews.appspot.com/805/diff/1/9
File user/src/com/google/gwt/user/client/Event.java (right):
http://gwt-code-reviews.appspot.com/805/diff/1/9#newcode187
Line 187: protected void kill() {
can you document the method that requires access to kill here?
http://gwt-code-reviews.appspot.com/805/diff/1/9#newcode482
Line 482: NativePreviewEvent event = new
NativePreviewEvent(nativeEvent);
I think we might want to recycle the native event here for efficiency.
http://gwt-code-reviews.appspot.com/805/diff/1/11
File user/src/com/google/gwt/user/client/ui/DialogBox.java (left):
http://gwt-code-reviews.appspot.com/805/diff/1/11#oldcode231
Line 231:
As much as I'd really, really love to do this, I think backward
compatibility forces us to deprecate the method rather then removing it.
http://gwt-code-reviews.appspot.com/805/diff/1/11
File user/src/com/google/gwt/user/client/ui/DialogBox.java (right):
http://gwt-code-reviews.appspot.com/805/diff/1/11#newcode392
Line 392:
do we want to expose this?
http://gwt-code-reviews.appspot.com/805/diff/1/13
File user/src/com/google/gwt/user/client/ui/MenuBar.java (left):
http://gwt-code-reviews.appspot.com/805/diff/1/13#oldcode966
Line 966: public boolean onEventPreview(Event event) {
See comments from DialogBox.
http://gwt-code-reviews.appspot.com/805/diff/1/13
File user/src/com/google/gwt/user/client/ui/MenuBar.java (right):
http://gwt-code-reviews.appspot.com/805/diff/1/13#newcode967
Line 967: protected void onPreviewNativeEvent(NativePreviewEvent event)
{
See comments from DialogBox.
http://gwt-code-reviews.appspot.com/805/diff/1/12
File user/src/com/google/gwt/user/client/ui/PopupPanel.java (right):
http://gwt-code-reviews.appspot.com/805/diff/1/12#newcode464
Line 464: */
Where is the onPreviewNativeEvent for this one?
http://gwt-code-reviews.appspot.com/805/diff/1/3
File user/test/com/google/gwt/user/client/EventTest.java (right):
http://gwt-code-reviews.appspot.com/805/diff/1/3#newcode26
Line 26: public class EventTest extends GWTTestCase {
Nice Test!
I think the only thing you need to add is the interactions of code that
mixes the old and new styles.
For example:
Use adds a new style preview, then an old style preview. What order
should they be evaluated in?
http://gwt-code-reviews.appspot.com/805
--~--~-~--~~~---~--~~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~--~~~~--~~--~--~---
