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.

Sign up for GitHub

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

Jump to bottom

BugFix Check emptiness of previousSortGroupState #12540

Merged
merged 3 commits into from
Feb 14, 2024
Merged

BugFix Check emptiness of previousSortGroupState #12540

merged 3 commits into from
Feb 14, 2024

Conversation

alperozturk96
Copy link
Collaborator

@alperozturk96 alperozturk96 commented Feb 12, 2024 •
edited
Loading

  • Tests written, or not not needed

@alperozturk96 alperozturk96 changed the title Check emptiness of previousSortGroupState BugFix Check emptiness of previousSortGroupState Feb 13, 2024
@github-actions GitHub Actions
Copy link

Codacy

Lint

TypemasterPR
Warnings6969
Errors33

SpotBugs

CategoryBaseNew
Bad practice6868
Correctness7373
Dodgy code357357
Experimental22
Internationalization77
Malicious code vulnerability22
Multithreaded correctness66
Performance5656
Security1818
Total589589

@github-actions GitHub Actions
Copy link

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/12540.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@alperozturk96
Copy link
Collaborator Author

/backport to stable-3.28

@codecov Codecov
Copy link

codecov bot commented Feb 14, 2024 •
edited
Loading

Codecov Report

Merging #12540 (480ad5f) into master (f7502ce) will decrease coverage by 0.06%.
Report is 34 commits behind head on master.
The diff coverage is 33.33%.

Additional details and impacted files 
@@             Coverage Diff              @@
##             master   #12540      +/-   ##
============================================
- Coverage     27.99%   27.94%   -0.06%     
+ Complexity     3368     3365       -3     
============================================
  Files           622      622              
  Lines         45514    45518       +4     
  Branches       6519     6521       +2     
============================================
- Hits          12742    12720      -22     
- Misses        30642    30660      +18     
- Partials       2130     2138       +8
Files Coverage Δ
...owncloud/android/ui/adapter/OCFileListAdapter.java 45.52% <100.00%> (+0.20%) ⬆️
...cloud/android/ui/activity/FileDisplayActivity.java 29.89% <0.00%> (-0.05%) ⬇️

... and 11 files with indirect coverage changes

ZetaTom
ZetaTom reviewed Feb 14, 2024
View reviewed changes
app/src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.java
@@ -2486,7 +2486,12 @@ private void setSortListGroup(boolean currentListGroupVisibility, boolean show)
* visibility earlier using {@link #setSortListGroup(boolean, boolean)}
*/
private void popSortListGroupVisibility() {
boolean popped = previousSortGroupState.pop();
showSortListGroup(popped);
showSortListGroup(false);
Copy link
Collaborator

@ZetaTom ZetaTom Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would negate purpose of previousSortGroupState, which was introduced by #11965. If this choice was deliberate, check that all edge cases are handled appropriately and remove this variable entirely.

Copy link
Collaborator Author

@alperozturk96 alperozturk96 Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current implementation on master is broken. We should't see SortListGroup in file detail or file sharing tab also we have crash right now. Crash has been fixed and appearance of SortListGroup working as expected (visible in file list not visible in file details, sharing).

This PR aims to fix crash and appearance of SortListGroup not refactor.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also agree with Tom, but we should create a new Issue about it and for now merge to fix the crash.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JonasMayerDev I agree

JonasMayerDev
JonasMayerDev approved these changes Feb 14, 2024
View reviewed changes
Copy link
Collaborator

@JonasMayerDev JonasMayerDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick test, fixes crash

app/src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.java
@@ -2486,7 +2486,12 @@ private void setSortListGroup(boolean currentListGroupVisibility, boolean show)
* visibility earlier using {@link #setSortListGroup(boolean, boolean)}
*/
private void popSortListGroupVisibility() {
boolean popped = previousSortGroupState.pop();
showSortListGroup(popped);
showSortListGroup(false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also agree with Tom, but we should create a new Issue about it and for now merge to fix the crash.

@alperozturk96 alperozturk96 merged commit dac1b46 into master Feb 14, 2024
24 checks passed
@delete-merged-branch delete-merged-branch bot deleted the bugfix/pop-sort-list-crash branch February 14, 2024 14:08
@backportbot backportbot bot mentioned this pull request Feb 14, 2024
Merged
@JonasMayerDev JonasMayerDev mentioned this pull request Feb 14, 2024
Closed
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.29.0 milestone Feb 14, 2024
@ZetaTom ZetaTom mentioned this pull request Mar 11, 2024
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZetaTom ZetaTom ZetaTom left review comments

@JonasMayerDev JonasMayerDev JonasMayerDev approved these changes

@tobiasKaminsky tobiasKaminsky Awaiting requested review from tobiasKaminsky

@AndyScherzinger AndyScherzinger Awaiting requested review from AndyScherzinger

Assignees
No one assigned
Labels
3. to review
Projects
None yet
Milestone
Nextcloud App 3.29.0
Development

Successfully merging this pull request may close these issues.
4 participants
@alperozturk96 @JonasMayerDev @ZetaTom @AndyScherzinger