Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add abstract layer for displaying elements #3

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add abstract layer for displaying elements #3

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2a9d684
Bump GWT to 2.9.0
gkresic Mar 8, 2021
0373d11
Switch to external gwt-widgets module
gkresic Mar 8, 2021
454df63
Abstracted widgets out of "core" activities
gkresic Mar 8, 2021
639def5
ElementalActivityDisplay for displaying HTMLElement inside HTMLElement
gkresic Mar 8, 2021
2443f40
Update README.md
gkresic Mar 8, 2021
570cb79
Classes and interfaces prespecialized to IsWidget/AcceptsOneWidget
gkresic Mar 9, 2021
f6e54c1
Merge branch 'display' of github.com:gkresic/gwt-activity into display
gkresic Mar 9, 2021
e585259
Properly initialize wrapped display in WidgetActivityManager
gkresic Mar 9, 2021
db48bc7
Propery declare override for WidgetActivityMapper.getActivity
gkresic Mar 9, 2021
476dda3
Simplified WidgetActivityManager
gkresic Mar 10, 2021
40fd8ef
Wrap ActivityManager in WidgetActivityManager
gkresic Mar 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@
<configuration>
<includes>
<include>org/gwtproject/activity/ActivityJreSuite.java</include>
<include>org/gwtproject/activity/widget/WidgetActivityJreSuite.java</include>
</includes>
</configuration>
</plugin>
Expand Down
27 changes: 27 additions & 0 deletions src/main/java/org/gwtproject/activity/widget/shared/AbstractWidgetActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright 2010 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package org.gwtproject.activity.widget.shared;

import org.gwtproject.activity.shared.AbstractActivity;
import org.gwtproject.user.client.ui.IsWidget;

/**
* Drop-in replacement for {@link com.google.gwt.activity.shared.AbstractActivity}.
*/
public abstract class AbstractWidgetActivity extends AbstractActivity<IsWidget>
implements WidgetActivity {

}
38 changes: 38 additions & 0 deletions src/main/java/org/gwtproject/activity/widget/shared/CachingWidgetActivityMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Copyright 2010 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package org.gwtproject.activity.widget.shared;

import org.gwtproject.activity.shared.CachingActivityMapper;
import org.gwtproject.place.shared.Place;
import org.gwtproject.user.client.ui.IsWidget;

/**
* Drop-in replacement for {@link com.google.gwt.activity.shared.CachingActivityMapper}.
*/
public class CachingWidgetActivityMapper extends CachingActivityMapper<IsWidget>
implements WidgetActivityMapper {

public CachingWidgetActivityMapper(WidgetActivityMapper wrapped) {
super(wrapped);
}

@Override
public WidgetActivity getActivity(Place place) {
// we are wrapping WidgetActivityMapper, so it's safe to cast
return (WidgetActivity) super.getActivity(place);
}

}
38 changes: 38 additions & 0 deletions src/main/java/org/gwtproject/activity/widget/shared/FilteredWidgetActivityMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Copyright 2010 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package org.gwtproject.activity.widget.shared;

import org.gwtproject.activity.shared.FilteredActivityMapper;
import org.gwtproject.place.shared.Place;
import org.gwtproject.user.client.ui.IsWidget;

/**
* Drop-in replacement for {@link com.google.gwt.activity.shared.FilteredActivityMapper}.
*/
public class FilteredWidgetActivityMapper extends FilteredActivityMapper<IsWidget>
implements WidgetActivityMapper {

public FilteredWidgetActivityMapper(Filter filter, WidgetActivityMapper wrapped) {
super(filter, wrapped);
}

@Override
public WidgetActivity getActivity(Place place) {
// we are wrapping WidgetActivityMapper, so it's safe to cast
return (WidgetActivity) super.getActivity(place);
}

}
35 changes: 35 additions & 0 deletions src/main/java/org/gwtproject/activity/widget/shared/WidgetActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright 2010 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package org.gwtproject.activity.widget.shared;

import org.gwtproject.activity.shared.Activity;
import org.gwtproject.activity.shared.ActivityDisplay;
import org.gwtproject.event.shared.EventBus;
import org.gwtproject.user.client.ui.AcceptsOneWidget;
import org.gwtproject.user.client.ui.IsWidget;

/**
* Drop-in replacement for {@link com.google.gwt.activity.shared.Activity}.
*/
public interface WidgetActivity extends Activity<IsWidget> {

default void start(ActivityDisplay<IsWidget> panel, EventBus eventBus) {
start((AcceptsOneWidget) panel::show, eventBus);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because that's what ActivityManager will call, it means the start(AcceptsOneWidget,EventBus) will receive a different AcceptsOnePanel each time it's started, even if it's actually the same underlying AcceptsOnePanel we passed to the ActivityManager. That panel will have been wrapped twice here: once to convert it to an ActivityDisplay in WidgetActivityManager, and then to converted it back to an AcceptsOneWidget here.
Maybe using an actual wrapper class and testing it here with instanceof would be better?

class AcceptsOneWidgetActivityDisplayWrapper {
  final AcceptsOneWidget panel;

  AcceptsOneWidgetActivityDisplayWrapper(AcceptsOneWidget panel) {
    this.panel = panel;
  }
}
AcceptsOneWidget panel;
if (display instanceof AcceptsOneWidgetActivityDisplayWrapper) {
  panel = ((AcceptsOneWidgetActivityDisplayWrapper) panel).panel;
} else {
  panel = (AcceptsOneWidget) display::show
}
start(panel, eventBus);

Unless we a) those multiple wrappers aren't a problem in practice (in terms of performance and/or code size) and b) don't care about the breaking change (any activity testing either the type of the panel or whether it's the same as in the previous start will break when migrated to this lib)

Copy link
Author

@gkresic gkresic Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said, I would choose simplicity over microoptimization any time, but like you pointed out, this may actually break activities if they compare panels (didn't occur to me they may), so I agree this is safer. Will do.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like activities never had an opportunity to compare panels they got, since ActivityManager wraps every display/panel with new ProtectedDisplay every time Activity.start is invoked (see ActivityManager.tryStart).

So, (double) wrapping seems inevitable.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And on the other hand, that simplifies the code a lot, as setDisplay(AcceptsOnWidget panel) can then just call setDisplay(panel == null ? null : panel::setWidget) (i.e. you can remove the widgetDisplay and wrapperDisplay)

}

void start(AcceptsOneWidget panel, EventBus eventBus);

}
48 changes: 48 additions & 0 deletions src/main/java/org/gwtproject/activity/widget/shared/WidgetActivityManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright 2010 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package org.gwtproject.activity.widget.shared;

import org.gwtproject.activity.shared.ActivityDisplay;
import org.gwtproject.activity.shared.ActivityManager;
import org.gwtproject.event.shared.EventBus;
import org.gwtproject.user.client.ui.AcceptsOneWidget;
import org.gwtproject.user.client.ui.IsWidget;

/**
* Drop-in replacement for {@link com.google.gwt.activity.shared.ActivityManager}.
*/
public class WidgetActivityManager extends ActivityManager<IsWidget> {

private AcceptsOneWidget widgetDisplay;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's never assigned, maybe it doesn't really matter if passing the same display should be a no-op or not (ActivityManager doesn't really care, all it cares about apparently is whether we go from null to non-null, or from non-null to null), but it's a breaking change.

This should be assigned in setDisplay, and tested for non-regression wrt c.g.g.activity.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's never assigned, maybe it doesn't really matter if passing the same display should be a no-op or not (ActivityManager doesn't really care, all it cares about apparently is whether we go from null to non-null, or from non-null to null), but it's a breaking change.

Indeed, I didn't see that. I saw it was comparing it (instance compare), so I took the safe route and decided to provide same wrapper for same delegate. If my 2 cents are worth anything, I wouldn't care about instancing new wrapper for each call since it results in much more readable code.

This should be assigned in setDisplay, and tested for non-regression wrt c.g.g.activity.

Yes, that was intention, but I forgot and since ActivityManager really doesn't care (like you pointed), it didn't matter in tests.

I'll initialize it, but will leave a note for possible simplification (with performance penalty, though).


private ActivityDisplay<IsWidget> wrapperDisplay;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't it be a local variable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, no, at least if we want to return same wrapper for each display. Note that it's initialized inside if, only if display is different from previous one. Otherwise, its left as an existing wrapper used in prior call.


public WidgetActivityManager(WidgetActivityMapper mapper, EventBus eventBus) {
super(mapper, eventBus);
}

public void setDisplay(AcceptsOneWidget display) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this extend or wrap?

  • by extending, this allows any ActivityDisplay<IsWidget>, but breaks setDisplay(null) that now requires a explicit cast to choose an overload
  • by wrapping, it cannot be used interchangeably with an ActivityManager<IsWidget> (similar to the CompatEventBus in gwt-event-compat), but ensures backwards compatibility

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like you pointed out, either one is bad. Javadoc could alleviate issue with required cast of nulls, but it's far from optimal. What is your suggestion?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would wrap, because I don't see a need for casting to ActivityManager<IsWidget>. And I would have a constructor that takes the ActivityManager<IsWidget> to be wrapped, and another constructor that would instantiate one, for ease of use. If one needs both an ActivityManager<IsWidget> and a WidgetActivityManager, they could then create the former and wrap it in the latter (similar to CompatEventBus)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapped.

I exposed all public methods from wrapped ActivityManager<IsWidget> together with wrapped object itself (via (getDelegateManager) and explained relationship to ActivityManager<IsWidget> in docs.

// since ActivityManager holds a reference to passed display,
// make sure we return same ActivityDisplay for same AcceptsOneWidget
if (display == null) {
wrapperDisplay = null;
} else if (display != widgetDisplay) {
wrapperDisplay = display::setWidget;
}
super.setDisplay(wrapperDisplay);
}

}
29 changes: 29 additions & 0 deletions src/main/java/org/gwtproject/activity/widget/shared/WidgetActivityMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* Copyright 2010 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package org.gwtproject.activity.widget.shared;

import org.gwtproject.activity.shared.ActivityMapper;
import org.gwtproject.place.shared.Place;
import org.gwtproject.user.client.ui.IsWidget;

/**
* Drop-in replacement for {@link com.google.gwt.activity.shared.ActivityMapper}.
*/
public interface WidgetActivityMapper extends ActivityMapper<IsWidget> {

WidgetActivity getActivity(Place place);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have an @Override

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.


}
30 changes: 30 additions & 0 deletions src/test/java/org/gwtproject/activity/widget/WidgetActivityJreSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright 2015 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy of
* the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations under
* the License.
*/
package org.gwtproject.activity.widget;

import org.gwtproject.activity.widget.shared.WidgetActivityManagerTest;
import org.junit.runner.RunWith;
import org.junit.runners.Suite;

/**
* Tests of the activity package.
*/
@RunWith(Suite.class)
@Suite.SuiteClasses({
WidgetActivityManagerTest.class
})
public class WidgetActivityJreSuite {
}
Loading