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.

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

Fix ERXDisplayGroup.setSelectedObjects() #678

Merged
merged 3 commits into from Jan 14, 2016
Merged

Conversation

schmied
Copy link
Contributor

@schmied schmied commented Sep 16, 2015

for display groups without a datasource, e.g. if specified via
setObjectArray(). Otherwise the selected objects get lost.

for display groups without a datasource, e.g. if specified via
setObjectArray(). Otherwise the selected objects get lost.
@darkv
Copy link
Member

darkv commented Sep 18, 2015

That will prevent the firing of events displayGroupShouldChangeSelectionToIndexes, displayGroupDidChangeSelectedObjects and displayGroupDidChangeSelection. Can you elaborate the problem you encountered?

@schmied
Copy link
Contributor Author

schmied commented Sep 18, 2015

I create a display group without a datasource and put objects via setObjectArray(). Then I select objects with setSelectedObjects(). If I switch the batch, the selected objects disappear. I do not know whether this behaviour is intended or not, but it behaved differently before your commit on this last year. We ship our bundled Wonder libraries with this patch without problems for over a year now. I do not know whether this fix is correct or if there is a better way to fix this. Thanks!

@darkv
Copy link
Member

darkv commented Sep 22, 2015

That seems the intended behavior of WODisplayGroup. Selections apparently are based on the displayed objects not all objects. At least that is what the API docs and the code are saying. You want to have a selection not tied to the currently displayed objects. Probably it would make more sense to create a separate "globalSelection" logic in ERXDisplayGroup for that, leaving the original expected behavior intact?

@schmied
Copy link
Contributor Author

schmied commented Sep 23, 2015

Then it seems there is an inconsistency in keeping / not keeping selected objects if changing batch via setCurrentBatchIndex(). Without a datasource the selected objects are gone (after your patch last year), with a datasource the objects are kept.

ERXDisplayGroup configured via Component.woo (i.e. with data source, master-detail):

displayGroup.setSelectedObjects(displayGroup.allObjects());
displayGroup.setCurrentBatchIndex(5); // still all objects selected
displayGroup.displayNextBatch(); // still all objects selected too

From my personal view selected objects should not interfer with displayed objects. This is the behaviour for display groups with data sources.

BTW the javadoc for setSelectedObjects() makes not much sense to me:
"Changes the WODisplayGroup's selection from displayedObjects to objects. Invokes setSelectionIndexes to do so."
Perhaps it should read:
"Changes the WODisplayGroup's selection from selectedObjects to objects. Invokes setSelectionIndexes to do so."

Or do they mean "out of displayedObjects"?

Am I missing something?

@spelletier
Copy link
Member

The doc for displayNextBatch() and displayPreviousBatch() state that the selection is cleared but the doc for setCurrentBatchIndex(int batchIndex) does not really.

Personally, I never used the selection because it did not behave in a coherent and logical way for me. If I had to suggest something, it would be the selection to remains unless not possible.

I just check the ERXDisplayGroup code and it seems to fix the WODisplayGroup problems, I should probably begin to use it's selection facility... All the methods in WODisplayGroup that previously cleared the selection are overridden to save the selection and restore it after calling the WODisplayGroup implementation. There is an clear intent to keep the selection in the ERXDisplayGroup code.

I think this fix should be merged as it restore a previous clearly expected behaviour.

I also suggest to add a comment on top of the class file to state this intent of keeping the selection.

@darkv
Copy link
Member

darkv commented Sep 25, 2015

That class is a real mess. Contradicting Javadocs and code and nowhere an in-depth documentation of that class and its usage :-/

Perhaps a way to solve this:

  • set the object array directly like in your use case. Then you are on your own and get no events and delegate usage.
  • use an EOArrayDataSource and set your object array there to get all events and delegate triggers.

If that would be a way to go we should clearly state that difference in the Javadocs: data sources for full features or direct arrays for … uh … something else ;-)

@darkv
Copy link
Member

darkv commented Oct 13, 2015

@schmied can you update the Javadocs accordingly? I will then merge this pull request into Wonder6 & 7.

darkv added a commit that referenced this pull request Jan 14, 2016
Fix ERXDisplayGroup.setSelectedObjects()
@darkv darkv merged commit be27b69 into wocommunity:master Jan 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants