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.

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

Show note #3121

Merged
merged 1 commit into from
Mar 20, 2019
Merged

Show note #3121

merged 1 commit into from
Mar 20, 2019

Conversation

tobiasKaminsky
Copy link
Member

@tobiasKaminsky tobiasKaminsky commented Oct 9, 2018

This shows a note, if available, in file detail sharing fragment.
As it is only shown if a note is available, it is out of the box backward compatible.

Requires PR for server: nextcloud/server#12978

@tobiasKaminsky tobiasKaminsky force-pushed the sendNote branch 2 times, most recently from 6f699a2 to 2630ab7 Compare October 24, 2018 08:41
@nextcloud nextcloud deleted a comment Oct 26, 2018
@nextcloud nextcloud deleted a comment Oct 26, 2018
@tobiasKaminsky tobiasKaminsky changed the base branch from sendNote to master October 26, 2018 06:43
@tobiasKaminsky tobiasKaminsky force-pushed the showNote branch 3 times, most recently from 584978c to bfada77 Compare December 6, 2018 07:46
@tobiasKaminsky tobiasKaminsky changed the base branch from master to showSharedUser December 6, 2018 07:46
@AndyScherzinger
Copy link
Member

AndyScherzinger commented Dec 6, 2018

@tobiasKaminsky corresponding lib change has been merged to master branch and jitpack build has been triggered and turned green, so jitpack lib master will ship the lib change now 👍

@nextcloud nextcloud deleted a comment Dec 6, 2018
@AndyScherzinger AndyScherzinger added the needs info Waiting for info from user(s). Issues with this label will auto-stale. label Jan 23, 2019
@AndyScherzinger
Copy link
Member

pinging @schiessle for feedback/input ❤️

@nextcloud nextcloud deleted a comment Feb 13, 2019
@tobiasKaminsky
Copy link
Member Author

2019-02-15-093135 image

Is the note "ShareAble" good enough to recognize? (both on Android / web UI)

@AndyScherzinger
Copy link
Member

Is the note "ShareAble" good enough to recognize? (both on Android / web UI)

I don't think so to be honest. It could maybe have the text file icon or something. Maybe @nextcloud/designers have a nice idea?

@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

❗ No coverage uploaded for pull request base (showSharedUser@191e12a). Click here to learn what that means.
The diff coverage is 18.18%.

@@               Coverage Diff                @@
##             showSharedUser   #3121   +/-   ##
================================================
  Coverage                  ?   6.18%           
  Complexity                ?       1           
================================================
  Files                     ?     317           
  Lines                     ?   30602           
  Branches                  ?    4397           
================================================
  Hits                      ?    1894           
  Misses                    ?   28422           
  Partials                  ?     286
Impacted Files Coverage Δ Complexity Δ
...ain/java/com/owncloud/android/db/ProviderMeta.java 88% <ø> (ø) 0 <0> (?)
...ncloud/android/ui/fragment/OCFileListFragment.java 0% <ø> (ø) 0 <0> (?)
...a/com/owncloud/android/utils/FileStorageUtils.java 12.3% <0%> (ø) 0 <0> (?)
...wncloud/android/providers/FileContentProvider.java 15.25% <0%> (ø) 0 <0> (?)
...android/ui/fragment/FileDetailSharingFragment.java 0% <0%> (ø) 0 <0> (?)
...in/java/com/owncloud/android/datamodel/OCFile.java 59.55% <100%> (ø) 0 <0> (?)
...loud/android/datamodel/FileDataStorageManager.java 10.74% <50%> (ø) 0 <0> (?)

@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #3121 into master will increase coverage by 0.05%.
The diff coverage is 4.34%.

@@             Coverage Diff             @@
##             master   #3121      +/-   ##
===========================================
+ Coverage      6.26%   6.31%   +0.05%     
  Complexity        1       1              
===========================================
  Files           318     318              
  Lines         31189   31214      +25     
  Branches       4467    4469       +2     
===========================================
+ Hits           1954    1972      +18     
- Misses        28942   28948       +6     
- Partials        293     294       +1
Impacted Files Coverage Δ Complexity Δ
...ain/java/com/owncloud/android/db/ProviderMeta.java 88% <ø> (ø) 0 <0> (ø) ⬇️
...loud/android/datamodel/ThumbnailsCacheManager.java 15.08% <ø> (ø) 0 <0> (ø) ⬇️
...ncloud/android/ui/fragment/FileDetailFragment.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...a/com/owncloud/android/utils/FileStorageUtils.java 8% <0%> (-0.05%) 0 <0> (ø)
...cloud/android/ui/adapter/FileDetailTabAdapter.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...owncloud/android/ui/adapter/OCFileListAdapter.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...wncloud/android/providers/FileContentProvider.java 18.9% <0%> (-0.23%) 0 <0> (ø)
...android/ui/fragment/FileDetailSharingFragment.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...in/java/com/owncloud/android/datamodel/OCFile.java 59.36% <100%> (+0.18%) 0 <0> (ø) ⬇️
...loud/android/datamodel/FileDataStorageManager.java 11.78% <50%> (+0.12%) 0 <0> (ø) ⬇️
... and 4 more

@nextcloud nextcloud deleted a comment Feb 16, 2019
@jancborchardt
Copy link
Member

Is the note "ShareAble" good enough to recognize? (both on Android / web UI)

I don't think so to be honest. It could maybe have the text file icon or something. Maybe @nextcloud/designers have a nice idea?

I’d say it’s good for the first pass – normally the note has some more explanatory content than just a word, and it’s inherently understandable as a note.

If we see it’s a problem in actual use, we can always add .icon-edit, the text document icon we also use for the "Add note" menu entry.

@tobiasKaminsky tobiasKaminsky changed the base branch from showSharedUser to master February 25, 2019 08:10
@nextcloud nextcloud deleted a comment Feb 25, 2019
@AndyScherzinger
Copy link
Member

@tobiasKaminsky needs a rebase and one question: is this ready to merge or does it need the server PR to be merged?

@tobiasKaminsky
Copy link
Member Author

@tobiasKaminsky needs a rebase and one question: is this ready to merge or does it need the server PR to be merged?

This originally depended on #3320, but I rebased it onto master, which now results in duplicated classes/additions, so I would like to merge #3320 before this.
After merge of #3320 I'll do a rebase.

This still requires nextcloud/server#12978 to be merged.
Backward compability should be working out of the box as older server just do not respond to "nc:note".
But I'll test it then.

(sorry for the confusion)

@tobiasKaminsky tobiasKaminsky added 3. to review and removed 2. developing needs info Waiting for info from user(s). Issues with this label will auto-stale. labels Feb 28, 2019
@nextcloud nextcloud deleted a comment Feb 28, 2019
@nextcloud nextcloud deleted a comment Feb 28, 2019
@@ -872,6 +873,16 @@ Drawable doAvatarInBackground() {

int px = getAvatarDimension();

boolean notOnSameServer = false;

if (notOnSameServer) {
Copy link
Member

Choose a reason for hiding this comment

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

@tobiasKaminsky this is always false...

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, why I did this.
Maybe to test…
I'll fix it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We ignore this for now. If there is a federated share, only the text generated avatar will be shown.
Ref: nextcloud/server#14564

@@ -1,5 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<!--
<?xml version="1.0" encoding="utf-8"?><!--
Copy link
Member

Choose a reason for hiding this comment

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

linebreak before the comment :)

@AndyScherzinger
Copy link
Member

@tobiasKaminsky just two minor comments plus needs-rebase :)

@AndyScherzinger
Copy link
Member

@tobiasKaminsky see review comments :)

@AndyScherzinger
Copy link
Member

@tobiasKaminsky rebase 'n' merge? ❤️

@tobiasKaminsky
Copy link
Member Author

Not quite…the part with federated icons is still open.
I hope I find time tomorrow :/

Signed-off-by: tobiasKaminsky <[email protected]>
@tobiasKaminsky
Copy link
Member Author

Finally done :-)

@nextcloud-android-bot
Copy link
Collaborator

Lint

TypemasterPR
Warnings6868
Errors00

FindBugs (new)

Warning TypeNumber
Bad practice Warnings33
Correctness Warnings118
Internationalization Warnings15
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings119
Security Warnings56
Dodgy code Warnings120
Total475

FindBugs (master)

Warning TypeNumber
Bad practice Warnings33
Correctness Warnings118
Internationalization Warnings15
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings119
Security Warnings56
Dodgy code Warnings120
Total475

@nextcloud nextcloud deleted a comment Mar 20, 2019
@AndyScherzinger AndyScherzinger merged commit 01acf65 into master Mar 20, 2019
@AndyScherzinger AndyScherzinger deleted the showNote branch March 20, 2019 09:01
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.6.0 milestone Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants