You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by "aminabromand (via GitHub)" <gi...@apache.org> on 2024/01/09 16:08:17 UTC
[PR] PartialPageUpdater could not update if different object references were used for its own page and the component to update [wicket]
aminabromand opened a new pull request, #762:
URL: https://github.com/apache/wicket/pull/762
fixed equality check of PartialPageUpdater to use equals(...) method instead of equals sign
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PartialPageUpdater could not update if different object references were used for its own page and the component to update [wicket]
Posted by "aminabromand (via GitHub)" <gi...@apache.org>.
aminabromand commented on code in PR #762:
URL: https://github.com/apache/wicket/pull/762#discussion_r1447225625
##########
wicket-core/src/main/java/org/apache/wicket/page/PartialPageUpdate.java:
##########
@@ -567,7 +567,7 @@ public final void add(final Component component, final String markupId)
LOG.warn("Component '{}' not cannot be updated because it was already removed from page", component);
return;
}
- else if (pageOfComponent != page)
+ else if (!pageOfComponent.equals(page))
Review Comment:
ok, understood.
Anyhow, thanks for the support and quick responses! 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PartialPageUpdater could not update if different object references were used for its own page and the component to update [wicket]
Posted by "aminabromand (via GitHub)" <gi...@apache.org>.
aminabromand commented on code in PR #762:
URL: https://github.com/apache/wicket/pull/762#discussion_r1447214792
##########
wicket-core/src/main/java/org/apache/wicket/page/PartialPageUpdate.java:
##########
@@ -567,7 +567,7 @@ public final void add(final Component component, final String markupId)
LOG.warn("Component '{}' not cannot be updated because it was already removed from page", component);
return;
}
- else if (pageOfComponent != page)
+ else if (!pageOfComponent.equals(page))
Review Comment:
I agree with you, that the current design of the application does not look that good.
But it is a legacy appliation, that I have inheritted and that I have just updated from Wicket 6.x to Wicket 9.x.
I was able to find a workaround:
instead of calling `target.add(button);`, where `target` is the `AjaxRequestTarget` in `@Override protected void onTimer(AjaxRequestTarget target) {...}` and `button` is the `Component` from the registry.
now I call `getButtonRef(target.getPage(), button.getPath()).ifPresentOrElse(target::add,() -> target.add(button));` with `getButtonRef()` =
`
private Optional<Component> getButtonRef(Page page, String buttonPath) {
String pagePath = page.getPath();
if (!buttonPath.startsWith(pagePath)) {
return Optional.empty();
}
buttonPath = buttonPath.substring(pagePath.length() + 1);
return Optional.ofNullable(page.get(buttonPath));
}
`
so I look for the child component of the page, that has the same `path` as the button in the registry.
Anyhow, I do think, doing the comparison with `equals()` instead of `==` has some advantages.
But I let you decide :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PartialPageUpdater could not update if different object references were used for its own page and the component to update [wicket]
Posted by "aminabromand (via GitHub)" <gi...@apache.org>.
aminabromand commented on code in PR #762:
URL: https://github.com/apache/wicket/pull/762#discussion_r1446631303
##########
wicket-core/src/main/java/org/apache/wicket/page/PartialPageUpdate.java:
##########
@@ -567,7 +567,7 @@ public final void add(final Component component, final String markupId)
LOG.warn("Component '{}' not cannot be updated because it was already removed from page", component);
return;
}
- else if (pageOfComponent != page)
+ else if (!pageOfComponent.equals(page))
Review Comment:
Sorry, I have missed the most important part in the pull request.
So now I have added another commit containing the `equals()` and `hashCode()` methods for the Page.class.
Well my usecase is the following:
I have a `AbstractAjaxTimerBehavior` on my page, which runs every 500 milli secs.
This `AbstractAjaxTimerBehavior` pulls Buttons out of a registry.
The registry contains the Buttons together with an information, whether the button should be enabled or disabled.
The state of the button enabled/disabled can be updated via a websocket, that is connected to the server.
So when the `AbstractAjaxTimerBehavior` pulls the Buttons our of the registry, the buttons are assigned to the same page, but it is not the very same object in the heap.
Thus the inequality check with `pageOfComponent != page` goes true, even though the page is the same.
This is why I would like to have the inequality check as `!pageOfComponent.equals(page)` with the `equals()` method checking on the immutal fields of the page.
Do you agree, that this is a sensible approach, or would you suggest otherwise?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PartialPageUpdater could not update if different object references were used for its own page and the component to update [wicket]
Posted by "svenmeier (via GitHub)" <gi...@apache.org>.
svenmeier commented on code in PR #762:
URL: https://github.com/apache/wicket/pull/762#discussion_r1447218599
##########
wicket-core/src/main/java/org/apache/wicket/page/PartialPageUpdate.java:
##########
@@ -567,7 +567,7 @@ public final void add(final Component component, final String markupId)
LOG.warn("Component '{}' not cannot be updated because it was already removed from page", component);
return;
}
- else if (pageOfComponent != page)
+ else if (!pageOfComponent.equals(page))
Review Comment:
Changing #equals and #hashCode in such a central class can have severe negative consequence, that I would not like to risk.
Your idea with updating via component path sounds good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PartialPageUpdater could not update if different object references were used for its own page and the component to update [wicket]
Posted by "aminabromand (via GitHub)" <gi...@apache.org>.
aminabromand closed pull request #762: PartialPageUpdater could not update if different object references were used for its own page and the component to update
URL: https://github.com/apache/wicket/pull/762
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PartialPageUpdater could not update if different object references were used for its own page and the component to update [wicket]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #762:
URL: https://github.com/apache/wicket/pull/762#discussion_r1447083054
##########
wicket-core/src/main/java/org/apache/wicket/page/PartialPageUpdate.java:
##########
@@ -567,7 +567,7 @@ public final void add(final Component component, final String markupId)
LOG.warn("Component '{}' not cannot be updated because it was already removed from page", component);
return;
}
- else if (pageOfComponent != page)
+ else if (!pageOfComponent.equals(page))
Review Comment:
> So when the `AbstractAjaxTimerBehavior` pulls the Buttons our of the registry, the buttons are assigned to the same page, but it is not the very same object in the heap.
So, you keep `Component`s (the buttons) in some registry ?
That would mean that you keep the buttons' parents in the registry, i.e. the whole page instances.
I am not sure this is a good idea! The items in the registry may become obsolete sooner or later depending on how much your application is based on SPA (single page application) design. I hope you have some cleanup mechanism!
IMO it would be better to have a registry with `ButtonDescription`s which are used to create Button components when needed and to replace the components in the current page instance before render.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PartialPageUpdater could not update if different object references were used for its own page and the component to update [wicket]
Posted by "svenmeier (via GitHub)" <gi...@apache.org>.
svenmeier commented on code in PR #762:
URL: https://github.com/apache/wicket/pull/762#discussion_r1446422367
##########
wicket-core/src/main/java/org/apache/wicket/page/PartialPageUpdate.java:
##########
@@ -567,7 +567,7 @@ public final void add(final Component component, final String markupId)
LOG.warn("Component '{}' not cannot be updated because it was already removed from page", component);
return;
}
- else if (pageOfComponent != page)
+ else if (!pageOfComponent.equals(page))
Review Comment:
Do you have a usecase, why this change should be necessary?
The check is precisely there to check on the page instance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PartialPageUpdater could not update if different object references were used for its own page and the component to update [wicket]
Posted by "aminabromand (via GitHub)" <gi...@apache.org>.
aminabromand commented on code in PR #762:
URL: https://github.com/apache/wicket/pull/762#discussion_r1447215766
##########
wicket-core/src/main/java/org/apache/wicket/Page.java:
##########
@@ -1012,4 +1013,21 @@ public final boolean wasRendered(Component component)
{
return renderedComponents != null && renderedComponents.contains(component);
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ Page that = (Page) o;
+ return autoIndex == that.autoIndex && numericId == that.numericId;
Review Comment:
you are right, that `autoIndex` is not good for the method as it can change.
But so does `renderCount`.
So I change the method to use `numericId` only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PartialPageUpdater could not update if different object references were used for its own page and the component to update [wicket]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #762:
URL: https://github.com/apache/wicket/pull/762#discussion_r1447067469
##########
wicket-core/src/main/java/org/apache/wicket/Page.java:
##########
@@ -1012,4 +1013,21 @@ public final boolean wasRendered(Component component)
{
return renderedComponents != null && renderedComponents.contains(component);
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ Page that = (Page) o;
+ return autoIndex == that.autoIndex && numericId == that.numericId;
Review Comment:
I think you should use `renderCount` instead of `autoIndex`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org