Skip to content

no sticky broadcast #6748

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
Merged
tobiasKaminsky merged 1 commit into from
Sep 2, 2020
Merged

no sticky broadcast #6748

tobiasKaminsky merged 1 commit into from
Sep 2, 2020

Conversation

@tobiasKaminsky
Copy link
Member

@tobiasKaminsky tobiasKaminsky commented Aug 19, 2020 •
edited
Loading

Fix #6654

  • use local broadcast system instead
  • no need to remove sticky broadcasts then

Signed-off-by: tobiasKaminsky tobias@kaminsky.me

Testing

Writing tests is very important. Please try to write some tests for your PR.
If you need help, please do not hesitate to ask in this PR for help.

unit tests
instrumented tests
UI tests

  • Tests written, or not not needed

@tobiasKaminsky tobiasKaminsky mentioned this pull request Aug 19, 2020
Closed
@codecov
Copy link

codecov bot commented Aug 21, 2020 •
edited
Loading

Codecov Report

Merging #6748 into master will increase coverage by 0.43%.
The diff coverage is 16.25%.

@@             Coverage Diff              @@
##             master    #6748      +/-   ##
============================================
+ Coverage     27.87%   28.30%   +0.43%     
  Complexity        5        5              
============================================
  Files           427      427              
  Lines         34270    34667     +397     
  Branches       4740     4847     +107     
============================================
+ Hits           9552     9812     +260     
- Misses        23294    23403     +109     
- Partials       1424     1452      +28
Impacted Files Coverage Δ Complexity Δ
...wncloud/android/files/services/FileDownloader.java 16.79% <0.00%> (ø) 0.00 <0.00> (ø)
...m/owncloud/android/services/SyncFolderHandler.java 9.37% <0.00%> (ø) 0.00 <0.00> (ø)
.../owncloud/android/syncadapter/FileSyncAdapter.java 4.23% <0.00%> (ø) 0.00 <0.00> (ø)
...roid/ui/activity/ReceiveExternalFilesActivity.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ncloud/android/ui/activity/UploadListActivity.java 4.95% <0.00%> (+0.11%) 0.00 <0.00> (ø)
...i/fragment/contactsbackup/ContactListFragment.java 0.98% <ø> (ø) 0.00 <0.00> (ø)
...cloud/android/ui/preview/PreviewImageActivity.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...cloud/android/ui/activity/FileDisplayActivity.java 24.46% <11.11%> (-1.19%) 0.00 <0.00> (ø)
...c/main/java/com/nextcloud/client/di/AppModule.java 85.29% <100.00%> (+0.44%) 0.00 <0.00> (ø)
.../owncloud/android/files/services/FileUploader.java 50.41% <100.00%> (+1.44%) 0.00 <0.00> (ø)
... and 14 more

ezaquarii
ezaquarii reviewed Aug 22, 2020
View reviewed changes
Copy link
Collaborator

@ezaquarii ezaquarii left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Some trivialities.

src/main/java/com/owncloud/android/ui/activity/FileDisplayActivity.java Outdated
updateActionBarTitleAndHomeButton(startFile);
}

localBroadCastManager = LocalBroadcastManager.getInstance(this);
Copy link
Collaborator

@ezaquarii ezaquarii Aug 22, 2020

Choose a reason for hiding this comment

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

This is nitpicking, but Broadcast instead of BroadCast. Sorry. :)

src/main/java/com/owncloud/android/ui/fragment/contactsbackup/ContactListFragment.java
import com.nextcloud.client.files.downloader.TransferState;
import com.nextcloud.client.files.downloader.TransferManagerConnection;
import com.nextcloud.client.files.downloader.Request;
import com.nextcloud.client.files.downloader.TransferState;
Copy link
Collaborator

@ezaquarii ezaquarii Aug 22, 2020

Choose a reason for hiding this comment

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

I think this is coming from one of my previous PRs that is already merged? Not sure how it ended up here.

tobiasKaminsky
tobiasKaminsky commented Aug 24, 2020
View reviewed changes
src/main/java/com/owncloud/android/operations/RefreshFolderOperation.java

intent.setPackage(mContext.getPackageName());
mContext.sendStickyBroadcast(intent);
LocalBroadcastManager.getInstance(mContext.getApplicationContext()).sendBroadcast(intent);
Copy link
Member Author

@tobiasKaminsky tobiasKaminsky Aug 24, 2020

Choose a reason for hiding this comment

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

@ezaquarii how could I use Dagger here?
Only via constructor, or?

Copy link
Collaborator

@ezaquarii ezaquarii Aug 24, 2020

Choose a reason for hiding this comment

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

Yeah, dagger works only in main components. Everything else is constructor based injection or setter if 2 stage init is required (I think we have 1 case of the latter).

Copy link
Member Author

@tobiasKaminsky tobiasKaminsky Aug 25, 2020

Choose a reason for hiding this comment

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

Do you think it is worth the effort, or can we keep it this way?
After all it is a singleton, so use of Dagger is not that much needed, but mainly only for centralizing the initiation?

Copy link
Member Author

@tobiasKaminsky tobiasKaminsky Aug 31, 2020

Choose a reason for hiding this comment

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

Let us keep it now this way to get it in faster.
If we need DI for testing, we can change it anyways.

@tobiasKaminsky
no sticky broadcast:
15dcad2 
- use local broadcast system instead
- no need to remove sticky broadcasts then

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@nextcloud-android-bot
Copy link
Collaborator

nextcloud-android-bot commented Aug 31, 2020

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/15642.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.

@nextcloud-android-bot
Copy link
Collaborator

nextcloud-android-bot commented Aug 31, 2020

Codacy

409

Lint

TypemasterPR
Warnings8181
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings27
Correctness Warnings61
Internationalization Warnings9
Multithreaded correctness Warnings9
Performance Warnings73
Security Warnings44
Dodgy code Warnings106
Total329

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings27
Correctness Warnings61
Internationalization Warnings9
Multithreaded correctness Warnings9
Performance Warnings73
Security Warnings44
Dodgy code Warnings106
Total329

@tobiasKaminsky tobiasKaminsky merged commit 47d66d6 into master Sep 2, 2020
@delete-merged-branch delete-merged-branch bot deleted the noStickyBroadcasts branch September 2, 2020 07:33
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.14.0 milestone Sep 2, 2020
@AndyScherzinger AndyScherzinger mentioned this pull request Nov 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezaquarii ezaquarii ezaquarii left review comments

@AndyScherzinger AndyScherzinger AndyScherzinger approved these changes

Assignees

No one assigned

Labels

3. to review

Projects

None yet

Milestone

Nextcloud App 3.14.0

Development

Successfully merging this pull request may close these issues.

Replace sticky broadcasts with and event-mechanism or else

5 participants

@tobiasKaminsky @nextcloud-android-bot @AndyScherzinger @ezaquarii