Buttonclass

[jay4kelly]

Closes #838

What has been done to verify that this works as intended?

Tested using UI for all buttons and chevrons.

Why is this the best possible solution? Were any other approaches considered?

Created classes called DetailsStatusButton and ExportConfigurationButton. The selection column didn't need a class for it as there was no business logic incorporated. So we continued to use a compareSelectionfunction inside the UI class.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Localized just to the UI tests conducted.

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

No

[ggalmazor]

Thanks, @jay4kelly! Bringing together all the code concerning those buttons helps a lot to have a tidy and well designed codebase. There's an issue about the use of extension in the new classes, though. Please, see my comments below.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@fd229a4). Click here to learn what that means.
The diff coverage is 7.95%.

@@            Coverage Diff            @@
##             master     #845   +/-   ##
=========================================
  Coverage          ?   48.29%           
  Complexity        ?     1637           
=========================================
  Files             ?      195           
  Lines             ?    10360           
  Branches          ?      747           
=========================================
  Hits              ?     5003           
  Misses            ?     5001           
  Partials          ?      356

Impacted Files	Coverage Δ	Complexity Δ
...briefcase/ui/reused/ExportConfigurationButton.java	0% <0%> (ø)	0 <0> (?)
...ase/ui/reused/transfer/TransferFormsTableView.java	0% <0%> (ø)	0 <0> (?)
...i/reused/transfer/TransferFormsTableViewModel.java	0% <0%> (ø)	0 <0> (?)
...takit/briefcase/ui/reused/DetailsStatusButton.java	0% <0%> (ø)	0 <0> (?)
src/org/opendatakit/briefcase/ui/reused/UI.java	45.16% <0%> (ø)	8 <0> (?)
...ndatakit/briefcase/ui/reused/ButtonProcessing.java	0% <0%> (ø)	0 <0> (?)
...i/export/components/ExportFormsTableViewModel.java	29.87% <28.57%> (ø)	9 <0> (?)
...ase/ui/export/components/ExportFormsTableView.java	85.1% <71.42%> (ø)	4 <0> (?)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd229a4...ff1d5d4. Read the comment docs.

[jay4kelly]

@ggalmazor Changed DetailStatusButton and ExportConfigurationButton to not extend JButton. Added ButtonProcessing Interface. Could have added a CommonButton class for the button classes to extend instead of using the interface. I think this would require us to remove the static create method on the button classes and use a new operator instead.

Going to review again

[ggalmazor]

Thanks, @jay4kelly! I like how you decided to extract an interface to provide polymorphic access to the wrapped JButton instance.

I think we should still fix a couple of things, though. Please, read my other comments.

[ggalmazor]

Here I think I would expect not having to unwrap the JButton object from the value, as in ((ButtonProcessing) value).click();

[jay4kelly]

Tried that but the doClick call was required to be performed by a JButton class. We can recheck that if you think it should work.

[ggalmazor]

I was thinking about adding a ButtonProcessing.doClick method that wraps button.doClick, to avoid breaking encapsulation.

[ggalmazor]

Hi, @jay4kelly!

After exploring a bit more the changes I was suggesting, I've come to the conclusion that they don't bring that much value overall.

This PR is looking good to me now :) Thanks for your work!!

[jay4kelly]

Closes #838

[kkrawczyk123]

Tested with success!
I haven't' noticed any regression. I have confirmed that the issue identified in #830 has been fixed.
Verified on Ubuntu, MacOS and Windows.

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"

[ggalmazor]

Hi, @jay4kelly! I've rebased your branch on top of master to be able to merge it. Thanks for your work!