getodk / briefcase

ODK Briefcase is a Java application for fetching and pushing forms and their contents. It helps make billions of data points from ODK portable. Contribute and make the world a better place! ✨💼✨
https://docs.getodk.org/briefcase-intro
Other
60 stars 154 forks source link

Jksortingissue #830

Closed jay4kelly closed 4 years ago

jay4kelly commented 4 years ago

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 commented 4 years ago

Codecov Report

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

Impacted file tree graph

@@             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> (ø) :arrow_down:
...i/export/components/ExportFormsTableViewModel.java 30.48% <100%> (ø) 10 <1> (ø) :arrow_down:
...ase/ui/export/components/ExportFormsTableView.java 85.71% <100%> (+0.93%) 4 <0> (ø) :arrow_down:
...g/opendatakit/briefcase/ui/export/ExportPanel.java 48.07% <0%> (ø) 14% <0%> (ø) :arrow_down:
...g/opendatakit/briefcase/reused/UncheckedFiles.java 46.21% <0%> (ø) 32% <0%> (+1%) :arrow_up:
...it/briefcase/push/central/CentralErrorMessage.java 0% <0%> (ø) 0% <0%> (ø) :arrow_down:

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 commented 4 years ago

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.

jay4kelly commented 4 years ago

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

kkrawczyk123 commented 4 years ago

@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: Peek 2019-12-18 10-21

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

jay4kelly commented 4 years ago

@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 commented 4 years ago

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

ggalmazor commented 4 years ago

@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 commented 4 years ago

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"