Jksortingissue

[jay4kelly]

Closes #331

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

Manual testing: Tested the three tabs(pull, push and export) using the sandbox aggregate server. Works as stated in the #331 issue. Test were performed on mac os and windows 10 intellij
gradlew ran to completion 390 succeded 6 ignored on mac os intellij.
Gradle failed on windows 10 (see details below)

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

Custom sorting in swing table using TableRowSorter typically use custom comparators for changing sort order. This solution is the standard one.

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?

Changes are described in the issue. The change is limited to when the column headers are clicked.

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

Ran into issue on windows 10 on intellij ide while running gradlew (verification -> check)
[ant:checkstyle] [ERROR] C:\Users\jay4k\IdeaProjects\briefcase\src\org\opendatakit\common\utils\WebUtils.java:182: Do not use Windows line endings [RegexpMultiline]

Task :checkstyleMain FAILED
FAILURE: Build failed with an exception.

• What went wrong:
  Execution failed for task ':checkstyleMain'.
  This happened for hundreds of lines on many files that were untouched by me.
  Tried altering git configuration and intellij configuration for end of line handling. Made sure codestyle for ide was the one supplied in the project.

On mac os intellij tests all run to completion. Unsure how to fix this on windows.

[codecov-io]

Codecov Report

Merging #830 into master will decrease coverage by 0.06%.
The diff coverage is 19.04%.

@@             Coverage Diff              @@
##             master     #830      +/-   ##
============================================
- Coverage     48.68%   48.62%   -0.07%     
  Complexity     1643     1643              
============================================
  Files           192      192              
  Lines         10314    10333      +19     
  Branches        743      746       +3     
============================================
+ Hits           5021     5024       +3     
- Misses         4935     4952      +17     
+ Partials        358      357       -1

Impacted Files	Coverage Δ	Complexity Δ
src/org/opendatakit/briefcase/ui/reused/UI.java	33.33% <0%> (-6.95%)	9 <0> (ø)
...ase/ui/reused/transfer/TransferFormsTableView.java	0% <0%> (ø)	0 <0> (ø)	⬇️
...i/export/components/ExportFormsTableViewModel.java	30.48% <100%> (ø)	10 <1> (ø)	⬇️
...ase/ui/export/components/ExportFormsTableView.java	85.71% <100%> (+0.93%)	4 <0> (ø)	⬇️
...g/opendatakit/briefcase/ui/export/ExportPanel.java	48.07% <0%> (ø)	14% <0%> (ø)	⬇️
...g/opendatakit/briefcase/reused/UncheckedFiles.java	46.21% <0%> (ø)	32% <0%> (+1%)	⬆️
...it/briefcase/push/central/CentralErrorMessage.java	0% <0%> (ø)	0% <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 a2941e0...3121a6b. Read the comment docs.

[ggalmazor]

Hi, @jay4kelly! Thanks for the PR. Before starting my review, I just wanted to tell you to not worry about the error you're getting when building in Windows. We have a CRLF vs LF situation in the checkstyle that's not important at the moment.

[ggalmazor]

Hi, @jay4kelly! Thanks for submitting the PR. I think we need to take care of some things before this PR can be merged. See my review comments.

I'd like to suggest another thing in case you're up for it. We could take advantage of the @FunctionalInterface of the Comparator<T> class and avoid having those three classes. Instead, we could have new methods in the UI class and pass method references to the sorter.setComparator() calls:

Example:

public class UI {

  // ...

  public static int compareConfButton(JButton o1, JButton o2) {
    if (o1.getForeground().equals(o2.getForeground()))
      return 0;
    if (o1.getForeground().equals(NO_CONF_OVERRIDE_COLOR) && o2.getForeground().equals(DARK_GRAY))
      return -1;
    return 1;
  }
}

And then, in ExportFormsTableView, do:

sorter.setComparator(OVERRIDE_CONF_COL, (Comparator<JButton>) UI::compareConfButton);

(notice that the cast of the reference is required to work around the Comparator<?> in the setComparator signature)

[jay4kelly]

Thanks for the review! I like your approach better. Made the appropriate changes.

[ggalmazor]

Thanks, @jay4kelly! This is looking good.

Now I realize that we have a potential problem with the loose coupling there is now with the colors we're using, and the semantic indirection there is between "having LIGHT_GRAY color" and something being "not active". I think we can merge this PR and document this in another issue and discuss options.

[kkrawczyk123]

@jay4kelly, In general, it looks very good but I have noticed a small issue that could be improved. On all tabs: I have used sorting by select checkbox and then by details column, when I select another form it is moved up to the list - so it looks like sorting by selection checkbox still works somehow.
after using select all and clear all the issue is still visible. I am attaching a gif:

cc @ggalmazor
@opendatakit-bot unlabel "needs testing"

[jay4kelly]

@kkrawczyk123
The selected column (checkbox) is sortable at the header as requested. There is also some automatic sorting that is enabled which I can turn off for the selected column (checkbox). That will resolve the behavior that you described. However, the details column for all three tabs is automatically sorted any time a push, pull or export is invoked. I appears that this behavior is intentional but let me know if those columns should not automatically sort. Manual sorting by clicking on the column headers would of course still be available.
cc @ggalmazor

[jay4kelly]

#845 Has been changed to include a fix for the automatic sorting mentioned by @kkrawczyk123

[ggalmazor]

@kkrawczyk123, I'm tagging this one for testing again in case you want to check the last change @jay4kelly added. Feel free to remove the tag and add behavior verified if you don't think it's necessary to retest it.

[kkrawczyk123]

The issue has been fixed in #845, so it will be verified there.
I can't see more issues, verified on Ubuntu, MacOS and Windows.

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