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

added copy options rules feature #7783

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aayushhyadav
Copy link
Contributor

related to #7767

@psiinon
Copy link
Member

psiinon commented Mar 15, 2023

Will the Duplicate option be valid for all such dialogs?
Might be better to have an allowDuplication option (as per allowModification) with the default as false.
That way we can enable it per dialog and then double check it works.
We dont really have any UI unit tests so we'd need to manually test these anyway...

@aayushhyadav
Copy link
Contributor Author

So will have to update AbstractMultipleOptionsBaseTablePanel( AbstractMultipleOptionsBaseTableModel<E> model, boolean allowModification) by adding another parameter of allowduplicate right? This currently has 3 usages, so do you mean that I should pass false from all 3 instances?

@aayushhyadav
Copy link
Contributor Author

Any idea how to resolve the build issue? I have tried to clean and build the project but the issue persists.

@thc202
Copy link
Member

thc202 commented Mar 24, 2023

You need to keep binary compatibility, e.g. add a new constructor instead of changing that one.

@aayushhyadav
Copy link
Contributor Author

Will the Duplicate option be valid for all such dialogs? Might be better to have an allowDuplication option (as per allowModification) with the default as false. That way we can enable it per dialog and then double check it works. We dont really have any UI unit tests so we'd need to manually test these anyway...

I have added the allowDuplication option and passed the same value as that of allowModification at all instances

@aayushhyadav
Copy link
Contributor Author

You need to keep binary compatibility, e.g. add a new constructor instead of changing that one.

I am trying to add something like protected AbstractMultipleOptionsBaseTablePanel(boolean allowDuplicate). From where should I invoke this constructor? I cannot use the this(false) from public AbstractMultipleOptionsBaseTablePanel(AbstractMultipleOptionsBaseTableModel<E> model) or protected AbstractMultipleOptionsBaseTablePanel(AbstractMultipleOptionsBaseTableModel<E> model, boolean allowModification) as they already use this and super respectively.

@aayushhyadav
Copy link
Contributor Author

One more query, can't we use the allowModification flag for duplicate functionality? So duplicate will only be available when modify is present. Or is it necessary to have them independent of each other?

@kingthorin
Copy link
Member

They should be independent. There may be situations where modifying one is fine but duplicating isn’t and vice versa.

@thc202
Copy link
Member

thc202 commented Mar 25, 2023

#7783 (comment)

You can change the implementation of the existing constructors just not their signature.

@aayushhyadav
Copy link
Contributor Author

#7783 (comment)

You can change the implementation of the existing constructors just not their signature.

Is the current approach fine?

@thc202
Copy link
Member

thc202 commented Mar 26, 2023

See #7783 (comment)

Signed-off-by: Aayush <aayushmum@gmail.com>
@aayushhyadav
Copy link
Contributor Author

Could anyone please review the changes? Are anymore changes required?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants