Addition of File Manager #816

askirk · 2018-07-16T19:50:26Z

Fixes (Part of) #782

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

Unit tests in this PR should cover all new code.

Summary

Uncomment FileManager sections in SdlManager
Implements agreed upon API for FileManager
Implements SdlFile, SdlArtwork

CLA

[ X ] I have signed the CLA

- Uncommented FileManager sections in SdlManager - Implement SdlFile and SdlArtwork classes - Implement methods in FileManager - Multiple file upload and deletion repeatedly calls completionListener, may be changed in future (ISdl)

…om/smartdevicelink/sdl_android into feature/issue_782_file_manager

- Replaced ArrayList type in member variable with List interface - Added missing calls to send RPC requests in setup and delete file

codecov-io · 2018-07-16T21:38:39Z

Codecov Report

Merging #816 into feature/issue_782_Android_Managers will increase coverage by 0.29%.
The diff coverage is 57.97%.

@@                           Coverage Diff                            @@
##             feature/issue_782_Android_Managers     #816      +/-   ##
========================================================================
+ Coverage                                 42.64%   42.93%   +0.29%     
- Complexity                                 2980     3028      +48     
========================================================================
  Files                                       376      379       +3     
  Lines                                     17729    17939     +210     
  Branches                                   1800     1832      +32     
========================================================================
+ Hits                                       7561     7703     +142     
- Misses                                     9816     9863      +47     
- Partials                                    352      373      +21

Impacted Files	Coverage Δ	Complexity Δ
...proxy/rpc/listeners/OnMultipleRequestListener.java	`40% <0%> (-2.86%)`	`3 <0> (+1)`
...n/java/com/smartdevicelink/proxy/SdlProxyBase.java	`4.73% <0%> (-0.01%)`	`6 <0> (ø)`
.../com/smartdevicelink/api/datatypes/SdlArtwork.java	`100% <100%> (ø)`	`5 <5> (?)`
...ava/com/smartdevicelink/api/datatypes/SdlFile.java	`100% <100%> (ø)`	`13 <13> (?)`
.../main/java/com/smartdevicelink/api/SdlManager.java	`50.29% <30%> (+2.19%)`	`24 <2> (+3)`	⬆️
...main/java/com/smartdevicelink/api/FileManager.java	`56.87% <56.87%> (ø)`	`23 <23> (?)`
... and 4 more

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 5d3decb...8a7824c. Read the comment docs.

- Multiple file and artwork upload/deletion now uses MultipleFileCompletionListener (which returns map of unsuccesful filenames and error messages) - ISdl now includes sendRequests (multiple RPCs asynch) - Refactored contentsOfResource and contentsOfUri methods to minimize code duplication - Updated FileManagerTests for new multiple upload and deletion methods

…om/smartdevicelink/sdl_android into feature/issue_782_file_manager # Conflicts: # sdl_android/src/main/java/com/smartdevicelink/api/SdlManager.java

- Added null check for ListFilesResponse - now transitions to ERROR state if initialization fails - new sendMultipleFileOperations avoids code duplication between uploadFiles and deleteRemoteFiles

- Added javadocs to all public methods in FileManager - Refactored tests to use Mockito apis, shrinking overhead and imports

bilal-alsharifi · 2018-07-20T17:47:27Z

sdl_android/src/main/java/com/smartdevicelink/api/SdlFile.java

@@ -0,0 +1,58 @@
+package com.smartdevicelink.api;
+
+import android.content.res.Resources;


This import can be removed

bilal-alsharifi · 2018-07-20T17:50:05Z

sdl_android/src/main/java/com/smartdevicelink/api/SdlArtwork.java

+                || fileType.equals(FileType.GRAPHIC_BMP)){
+            super.setType(fileType);
+        }else{
+            throw new IllegalArgumentException("Only JPEG and PNG image types are supported.");


Also BMP is supported

bilal-alsharifi · 2018-07-20T17:58:27Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+	 * @param listener callback that is called on response from core
+	 */
+	public void deleteRemoteFileWithName(final String fileName, final CompletionListener listener){
+		if(fileName == null){


Probably we should call listener.onComplete(false) here to let the developer know what happened? and also in deleteRemoteFilesWithNames(), uploadFiles()

I think if they don't provide a file or the list is empty, it's pretty much on them. They shouldn't be expecting a callback at that point

Yeah, right I agree.

bilal-alsharifi · 2018-07-20T18:15:09Z

sdl_android/src/main/java/com/smartdevicelink/proxy/SdlProxyBase.java

@@ -315,6 +315,15 @@ public void sendRPCRequest(RPCRequest message){
 			}
 		}

+		@Override


Not sure why we have to add this public method since there is already one that does that in the same class.

We need to implement this new method in ISdl to create the _internalInterface that is used by SdlManager and submanagers. See how sendRPCRequest() does it

Oh you are right. I did not realize that the new method is inside Isdl.

bilal-alsharifi · 2018-07-23T14:34:07Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+	 * @param file SdlArtwork with file name and one of A) fileData, B) Uri, or C) resourceID set
+	 * @param listener called when core responds to the attempt to upload the file
+	 */
+	public void uploadArtwork(final SdlArtwork file, final CompletionListener listener){


I think that uploadArtwork() & uploadArtworks() are unnecessary and can be removed. Because we can use uploadFile() & uploadFiles() to handle both SDLFile & SDLArtWork

The iOS library has these APIs so if we can stay consistent that is better.

Ahh okay, it makes sense.

-Remove unused import -Fix error message in SdlArtwork -Add null checks in FileManager apis

BrettyWhite · 2018-07-23T20:46:41Z

@askirk SdlFile and SdlArtwork should probably go in the same package as the other structs/objects and have tests like the other structs

Also added description for each class

- Add method and supporting list structure in FileManager - Add corresponding unit test - Fix logic where remoteFiles list was not being updated on multiple delete file operation. Add unit test to cover such a case

joeygrover · 2018-07-26T14:29:08Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+ */
+public class FileManager extends BaseSubManager {
+
+	private static String TAG = "FileManager";


Please add final modifier.

joeygrover · 2018-07-26T14:29:19Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+
+	private static String TAG = "FileManager";
+	private List<String> remoteFiles, uploadedEphemeralFileNames;
+	private WeakReference<Context> context;


Please add final modifier.

joeygrover · 2018-07-26T14:31:32Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+					}
+					// on callback set manager to ready state
+					transitionToState(BaseSubManager.READY);
+				}else{


This case will actually never happen. onResponse is only called when the response was successful. There is another method to override onError that is called when issues occur.

joeygrover · 2018-07-26T14:40:28Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+	 * @param fileName name of file to be deleted
+	 * @param listener callback that is called on response from core
+	 */
+	public void deleteRemoteFileWithName(final String fileName, final CompletionListener listener){


Please add the @nonnull tag to the fileName param

joeygrover · 2018-07-26T14:40:59Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+	 * @param listener callback that is called on response from core
+	 */
+	public void deleteRemoteFileWithName(final String fileName, final CompletionListener listener){
+		if(fileName == null || listener == null){


Does it actually matter if the listener is null? Can't the developer just request the file be deleted without caring if it was successful?

joeygrover · 2018-07-26T14:43:34Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+	 * @param fileNames list of file names to be deleted
+	 * @param listener callback that is called once core responds to all deletion requests
+	 */
+	public void deleteRemoteFilesWithNames(List<String> fileNames, final MultipleFileCompletionListener listener){


Please add the @nonnull tag to the fileNames param

joeygrover · 2018-07-26T14:43:53Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+	 * @param listener callback that is called once core responds to all deletion requests
+	 */
+	public void deleteRemoteFilesWithNames(List<String> fileNames, final MultipleFileCompletionListener listener){
+		if(fileNames == null || fileNames.isEmpty() || listener == null){


Same question above about the listener being null

joeygrover · 2018-07-26T14:44:32Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+	 * @param file SdlFile with fileName and one of A) fileData, B) Uri, or C) resourceID set
+	 * @return a valid PutFile request if SdlFile contained a fileName and sufficient data
+	 */
+	private PutFile createPutFile(final SdlFile file){


Please add the @nonnull tag to the file param

joeygrover · 2018-07-26T14:49:10Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+	private void sendMultipleFileOperations(final List<? extends RPCRequest> requests, final MultipleFileCompletionListener listener){
+		final Map<String, String> errors = new HashMap<>();
+		final SparseArray<String> fileNameMap = new SparseArray<>();
+		final Boolean deletionOperation;


This can be a primitive boolean

joeygrover · 2018-07-26T14:53:58Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+	 * @param file SdlFile with file name and one of A) fileData, B) Uri, or C) resourceID set
+	 * @param listener called when core responds to the attempt to upload the file
+	 */
+	public void uploadFile(final SdlFile file, final CompletionListener listener){


Please add the @nonnull tag to the file param

joeygrover · 2018-07-26T14:54:12Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+	 * @param listener called when core responds to the attempt to upload the file
+	 */
+	public void uploadFile(final SdlFile file, final CompletionListener listener){
+		if(file == null || listener == null){


Same question about the listener being null as above

joeygrover · 2018-07-26T14:58:21Z

sdl_android/src/main/java/com/smartdevicelink/api/FileManager.java

+	 * @param file SdlFile
+	 * @return boolean that tells whether file has been uploaded to core (true) or not (false)
+	 */
+	public boolean hasUploadedFile(SdlFile file){


Please add the @nonnull tag to the file param

joeygrover · 2018-07-26T15:02:18Z

Please pull in the feature/issue_782_Android_Managers to resolve conflicts

- Add NonNull tags where appropriate - Add some final keywords to applicable variables - No longer return early if completionListener is null, just don’t attempt to call it - Fix test case that used null parameter

…om/smartdevicelink/sdl_android into feature/issue_782_file_manager # Conflicts: # sdl_android/src/main/java/com/smartdevicelink/api/SdlManager.java

- Also moved PermissionManagerTests to api folder

joeygrover · 2018-07-26T19:33:27Z

...android/src/main/java/com/smartdevicelink/proxy/rpc/listeners/OnMultipleRequestListener.java

@@ -21,10 +21,22 @@
 	public OnMultipleRequestListener(){
 		setListenerType(UPDATE_LISTENER_TYPE_MULTIPLE_REQUESTS);
 		correlationIds = new Vector<>();
+		final OnMultipleRequestListener reference = this;


This is unnecessary. Just call OnMultipleRequestListener.this.method

joeygrover · 2018-07-26T19:34:27Z

...android/src/main/java/com/smartdevicelink/proxy/rpc/listeners/OnMultipleRequestListener.java

+				update(correlationId);
+			}
+
+			private void update(int correlationId){


This should be synchronized to make it thread safe

joeygrover · 2018-07-26T19:35:34Z

...android/src/main/java/com/smartdevicelink/proxy/rpc/listeners/OnMultipleRequestListener.java

+				update(correlationId);
+			}
+
+			private void update(int correlationId){
 				correlationIds.remove(Integer.valueOf(correlationId));


Why not just change the vector to int instead of Integer?

Vector can't use primitive type

askirk added 3 commits July 16, 2018 12:15

Basic functionality for FileManager methods

8d1a3c8

- Uncommented FileManager sections in SdlManager - Implement SdlFile and SdlArtwork classes - Implement methods in FileManager - Multiple file upload and deletion repeatedly calls completionListener, may be changed in future (ISdl)

Merge branch 'feature/issue_782_Android_Managers' of https://github.c…

92e11a9

…om/smartdevicelink/sdl_android into feature/issue_782_file_manager

Add unit tests for full coverage

e7d1cdc

- Replaced ArrayList type in member variable with List interface - Added missing calls to send RPC requests in setup and delete file

askirk requested review from bilal-alsharifi, BrettyWhite and joeygrover July 16, 2018 19:50

askirk added 2 commits July 16, 2018 16:03

Remove unused code in ISdl

940d78c

Create proxy before initialization of sub-managers

b908dbb

askirk added 5 commits July 17, 2018 16:31

Override OnError in MultipleRequestListener and PutFile

7ecc3a0

Merge branch 'feature/issue_782_Android_Managers' of https://github.c…

1268160

…om/smartdevicelink/sdl_android into feature/issue_782_file_manager # Conflicts: # sdl_android/src/main/java/com/smartdevicelink/api/SdlManager.java

Update for new ERROR state, refactor common code

09a1efa

- Added null check for ListFilesResponse - now transitions to ERROR state if initialization fails - new sendMultipleFileOperations avoids code duplication between uploadFiles and deleteRemoteFiles

Add javadocs and modify tests to use Mockito

74f7fed

- Added javadocs to all public methods in FileManager - Refactored tests to use Mockito apis, shrinking overhead and imports

bilal-alsharifi reviewed Jul 20, 2018

View reviewed changes

bilal-alsharifi reviewed Jul 23, 2018

View reviewed changes

Addressing review comments

18369ec

-Remove unused import -Fix error message in SdlArtwork -Add null checks in FileManager apis

bilal-alsharifi approved these changes Jul 23, 2018

View reviewed changes

askirk added 2 commits July 24, 2018 10:17

Add package to hold SdlFile, SdlArtwork

f484f84

Also added description for each class

Add hasUploadedFile() method to match iOS

dba373e

- Add method and supporting list structure in FileManager - Add corresponding unit test - Fix logic where remoteFiles list was not being updated on multiple delete file operation. Add unit test to cover such a case

joeygrover reviewed Jul 26, 2018

View reviewed changes

askirk added 3 commits July 26, 2018 12:46

Update based on review comments

aeb260f

- Add NonNull tags where appropriate - Add some final keywords to applicable variables - No longer return early if completionListener is null, just don’t attempt to call it - Fix test case that used null parameter

Merge branch 'feature/issue_782_Android_Managers' of https://github.c…

ec1af8e

…om/smartdevicelink/sdl_android into feature/issue_782_file_manager # Conflicts: # sdl_android/src/main/java/com/smartdevicelink/api/SdlManager.java

Fixing test, adding SdlManager state setter

373d89b

- Also moved PermissionManagerTests to api folder

joeygrover reviewed Jul 26, 2018

View reviewed changes

Remove local variable and synchronize method

8a7824c

joeygrover merged commit 9fd0f38 into feature/issue_782_Android_Managers Jul 26, 2018

joeygrover deleted the feature/issue_782_file_manager branch July 26, 2018 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addition of File Manager #816

Addition of File Manager #816

askirk commented Jul 16, 2018

codecov-io commented Jul 16, 2018 •

edited

bilal-alsharifi Jul 20, 2018

bilal-alsharifi Jul 20, 2018

bilal-alsharifi Jul 20, 2018

askirk Jul 23, 2018

bilal-alsharifi Jul 23, 2018

bilal-alsharifi Jul 20, 2018

askirk Jul 23, 2018

bilal-alsharifi Jul 23, 2018 •

edited

bilal-alsharifi Jul 23, 2018

joeygrover Jul 23, 2018

bilal-alsharifi Jul 23, 2018

BrettyWhite commented Jul 23, 2018 •

edited

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover commented Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

joeygrover Jul 26, 2018

askirk Jul 26, 2018

		@@ -0,0 +1,58 @@
		package com.smartdevicelink.api;

		import android.content.res.Resources;

Addition of File Manager #816

Addition of File Manager #816

Conversation

askirk commented Jul 16, 2018

Risk

Testing Plan

Summary

CLA

codecov-io commented Jul 16, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bilal-alsharifi Jul 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrettyWhite commented Jul 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeygrover commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 16, 2018 •

edited

bilal-alsharifi Jul 23, 2018 •

edited

BrettyWhite commented Jul 23, 2018 •

edited