Direct editing support #4890

tobiasKaminsky · 2019-11-26T10:31:52Z

abstract EditorWebView
support direct editing endpoint
needs Text app
behaviour:
if downloaded show it locally, use edit for remote editing
if not downloaded, open it remote directly

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

ezaquarii

just some random rambling

ezaquarii · 2019-11-26T21:44:03Z

src/main/java/com/nextcloud/client/di/ComponentsModule.java

    @ContributesAndroidInjector abstract ReceiveExternalFilesActivity receiveExternalFilesActivity();
    @ContributesAndroidInjector abstract RequestCredentialsActivity requestCredentialsActivity();
-    @ContributesAndroidInjector abstract RichDocumentsWebView richDocumentsWebView();
+


Whitespace? Not that I care, but it's editable in column mode this way :)

Damn AndroidStudio is reformatting it…

ezaquarii · 2019-11-26T22:05:08Z

src/main/java/com/owncloud/android/datamodel/FileDataStorageManager.java

        cv.put(ProviderTableMeta.CAPABILITIES_RICHDOCUMENT_TEMPLATES, capability.getRichDocumentsTemplatesAvailable()
            .getValue());
        cv.put(ProviderTableMeta.CAPABILITIES_RICHDOCUMENT_PRODUCT_NAME, capability.getRichDocumentsProductName());
+        cv.put(ProviderTableMeta.CAPABILITIES_DIRECT_EDITING, new Gson().toJson(capability.getDirectEditing()));


Danger! Crevasses!

IIRC Gson had some serious issues around Kotlin data classes. Can't remember what it was exactly, but I'll do some research and circle back. null handling was an issue for sure.

In the meantime, I'd seal that behind an interface and inject JSON serializer via Dagger top-down. I know it's not pretty, but we might need to revisit our JSON serializer choice in the near future and swap it out when we discover some funky behaviors. This area is full of nasty surprises, such as deserializers that use non-List-conforming lists and similar plesantries.

The upside is that we can supply serializer/deserializer with our custom type adapters (Date :) ) that will behave consistently across all modules.

Please don't ask which one will work. I don't know - this area is changing like frontend frameworks, with new library of the quater every month. (-: I'm just sharing my past pains here.

We are using Gson() in many other places.
Let us address this entirely (if needed) in a new issue / PR.

ezaquarii · 2019-11-26T22:07:52Z

src/main/java/com/owncloud/android/files/FileMenuFilter.java

+     * This will be replaced by unified editor and can be removed once EOL of corresponding server version.
+     */
+    @NextcloudServer(max = 18)
+    private boolean checkRichDocumentRequirement(OCCapability capability, String mimeType) {


Not sure what it checks for? isRichDocumentEditingSupported()?

ezaquarii · 2019-11-26T22:09:52Z

src/main/java/com/owncloud/android/ui/activity/EditorWebView.java

@@ -0,0 +1,228 @@
+/*


kt maybe?

ezaquarii · 2019-11-26T22:12:23Z

src/main/java/com/owncloud/android/ui/asynctasks/LoadUrlTask.java

 * along with this program. If not, see <https://www.gnu.org/licenses/>.
 */
 package com.owncloud.android.ui.asynctasks;



Having this one so simplified, I'd also consider converting it to a function and using AsyncRunner. This way this can be covered with tests. Async tasks - not so much.

ezaquarii · 2019-11-26T22:14:25Z

src/main/java/com/owncloud/android/ui/asynctasks/LoadUrlTask.java

+        }
+
+        RemoteOperationResult result = null;
+        if (editorWebViewWeakReference.get() instanceof RichDocumentsEditorWebView) {


editorWebViewWeakReference can become null between line 52 and 53.

Suggested change

if (editorWebViewWeakReference.get() instanceof RichDocumentsEditorWebView) {

final editorWebViewWeak = editorWebViewWeakReference.get(); // lock reference so it's not GC'd between lines

if (editorWebViewWeak == null) {

return;

}

and so on

Just discovered suggestions feature. Neat!

ezaquarii · 2019-11-26T22:16:11Z

src/main/java/com/owncloud/android/ui/fragment/OCFileListFragment.java

 import org.greenrobot.eventbus.EventBus;
 import org.greenrobot.eventbus.Subscribe;
 import org.greenrobot.eventbus.ThreadMode;
+import org.jetbrains.annotations.NotNull;


Watch out for proprietary annotation!

Nice finding!
This was auto-picked by Android Studio.
I will have a look for other usages within our project and do a PR then.

ezaquarii · 2019-11-26T22:17:22Z

src/main/java/com/owncloud/android/ui/fragment/OCFileListFragment.java

-                case R.id.action_open_file_as_richdocument: {
-                    mContainerActivity.getFileOperationsHelper().openFileAsRichDocument(singleFile, getContext());
+                case R.id.action_edit: {
+                    Account account = ((FileActivity) mContainerActivity).getAccount();


getUser? You can plug any rabbit holes with toPlatformAccount() - I'll scoop them.

AndyScherzinger · 2019-11-27T21:00:26Z

src/main/java/com/owncloud/android/operations/RichDocumentsUrlOperation.java

 public class RichDocumentsUrlOperation extends RemoteOperation {
+
+    /**
+     * TODO move to library


separate issue or move within this PR?

This will be outdated once we switch entire RichDocuments to use new DirectEditing endpoint (just like Text).
I annotate it with EOL 18 and we keep it here, as otherwise other library users might use it.

AndyScherzinger · 2019-11-27T21:01:14Z

src/main/java/com/owncloud/android/ui/activity/ExternalSiteWebView.java

        webview.setFocusable(true);
        webview.setFocusableInTouchMode(true);
        webview.setClickable(true);
+//        webview.addJavascriptInterface(new TestMobileInterface(), "RichDocumentsMobileInterface");


remove then?

AndyScherzinger · 2019-11-27T21:01:39Z

src/main/java/com/owncloud/android/ui/activity/RichDocumentsEditorWebView.java

-public class RichDocumentsWebView extends ExternalSiteWebView {
+public class RichDocumentsEditorWebView extends EditorWebView {
+    /**
+     * TODO: create intermediate abstract class, which has "loadURL", create this and TextWebView as children


this PR or separate issue?

This is what EditorWebView is doing :-)

AndyScherzinger · 2019-11-27T21:04:31Z

src/main/java/com/owncloud/android/ui/activity/RichDocumentsEditorWebView.java

+            int placeholder;
+
+            switch (template.getType()) {
+                case "document":


case strings should be defined as constants

AndyScherzinger · 2019-11-27T21:04:57Z

src/main/java/com/owncloud/android/ui/activity/TextEditorWebView.kt

@@ -0,0 +1,38 @@
+/*
+ *


line can e removed

AndyScherzinger · 2019-11-27T21:06:32Z

src/main/java/com/owncloud/android/ui/asynctasks/LoadUrlTask.java

+            result = new RichDocumentsUrlOperation(fileId[0]).execute(account, editorWebViewWeakReference.get());
+        } else if (editorWebViewWeakReference.get() instanceof TextEditorWebView) {
+            result = new DirectEditingOpenFileRemoteOperation(fileId[0],
+                                                              "text")


should be a constant

AndyScherzinger · 2019-11-27T21:09:37Z

Besides the exisitng review comments, just some minor issues regarding use of constants.

@tobiasKaminsky needs a rebase, db cange will conflict with #4784 (whichever makes it to master first)

codecov · 2019-12-02T12:16:48Z

Codecov Report

Merging #4890 into master will decrease coverage by 0.07%.
The diff coverage is 14.85%.

@@             Coverage Diff              @@
##             master    #4890      +/-   ##
============================================
- Coverage     17.94%   17.87%   -0.08%     
  Complexity        3        3              
============================================
  Files           385      389       +4     
  Lines         32679    32887     +208     
  Branches       4594     4620      +26     
============================================
+ Hits           5864     5877      +13     
- Misses        25872    26064     +192     
- Partials        943      946       +3

Impacted Files	Coverage Δ	Complexity Δ
...droid/ui/fragment/OCFileListBottomSheetDialog.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...java/com/nextcloud/client/di/ComponentsModule.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...cloud/android/datamodel/ArbitraryDataProvider.java	`67.9% <ø> (ø)`	`0 <0> (ø)`	⬇️
...ain/java/com/owncloud/android/db/ProviderMeta.java	`84.61% <ø> (ø)`	`0 <0> (ø)`	⬇️
...owncloud/android/ui/asynctasks/PrintAsyncTask.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...cloud/android/ui/activity/ExternalSiteWebView.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...owncloud/android/files/FetchTemplateOperation.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
...cloud/android/ui/helpers/FileOperationsHelper.java	`2.34% <0%> (-0.07%)`	`0 <0> (ø)`
...ncloud/android/ui/fragment/OCFileListFragment.java	`25.3% <0%> (-0.56%)`	`0 <0> (ø)`
...ndroid/ui/dialog/ChooseTemplateDialogFragment.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
... and 30 more

nextcloud-android-bot · 2019-12-05T06:23:50Z

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/11863

nextcloud-android-bot · 2019-12-05T09:49:44Z

IT test failed: https://www.kaminsky.me/nc-dev/android-integrationTests/11866

AndyScherzinger · 2019-12-09T09:14:40Z

@tobiasKaminsky looks good, the unused variable should be removed and spotbugs might be fine after a rebase(?).

elpraga · 2019-12-09T10:17:02Z

I've just tried the above app with nc17, and it did not offer online md editing. Do I need st to switch it on on the server side?

AndyScherzinger · 2019-12-09T10:45:58Z

needs Text app

elpraga · 2019-12-09T13:19:55Z

needs Text app

Thank you for your answer Andy, but I do have the text app installed in version 1.1.1.

AndyScherzinger · 2019-12-09T14:54:54Z

Then @tobiasKaminsky might need to do some bugfixing

tobiasKaminsky · 2019-12-09T15:05:21Z

Sorry for not being clear: this needs text app master, and NC18.

elpraga · 2019-12-09T22:46:24Z

Sorry for not being clear: this needs text app master, and NC18.

Then it's settled! Thank you for clarifying Tobias! :)

- abstract EditorWebView - support direct editing endpoint Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

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

…ity reason of javascript interface Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

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

- abstract EditorWebView - support direct editing endpoint Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

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

nextcloud-android-bot · 2019-12-19T08:13:53Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 8
- Added 4
           

Complexity increasing per file
==============================
- src/main/java/com/owncloud/android/ui/activity/RichDocumentsEditorWebView.java  5
- src/main/java/com/owncloud/android/providers/FileContentProvider.java  6
- src/main/java/com/owncloud/android/ui/preview/PreviewTextStringFragment.java  3
- src/main/java/com/owncloud/android/ui/asynctasks/LoadUrlTask.java  3
- src/main/java/com/owncloud/android/ui/preview/PreviewTextFileFragment.java  11
- src/main/java/com/owncloud/android/ui/activity/EditorWebView.java  13
- src/main/java/com/owncloud/android/ui/fragment/OCFileListFragment.java  2
- src/main/java/com/owncloud/android/ui/adapter/OCFileListAdapter.java  1
         

Complexity decreasing per file
==============================
+ src/main/java/com/owncloud/android/ui/preview/PreviewTextFragment.java  -6

See the complete overview on Codacy

nextcloud-android-bot · 2019-12-19T08:14:00Z

src/main/java/com/owncloud/android/ui/preview/PreviewTextFileFragment.java

+            try {
+                File file = new File(location);
+                reader = ReaderFactory.createReaderFromFile(file);
+                scanner = new Scanner(reader);


Issue found: Assigning an Object to null is a code smell. Consider refactoring.

src/main/java/com/owncloud/android/ui/fragment/OCFileListFragment.java

nextcloud-android-bot · 2019-12-19T08:14:03Z

src/main/java/com/owncloud/android/ui/preview/PreviewTextStringFragment.java

+     * <p>
+     * DO NOT CALL IT: an {@link OCFile} and {@link Account} must be provided for a successful construction
+     */
+    public PreviewTextStringFragment() {


Issue found: Avoid unnecessary constructors - the compiler will generate these for you

nextcloud-android-bot · 2019-12-19T08:14:04Z

src/main/java/com/owncloud/android/ui/preview/PreviewTextStringFragment.java

+        View view = super.onCreateView(inflater, container, savedInstanceState);
+
+        if (view == null) {
+            throw new RuntimeException("View may not be null");


Issue found: Avoid throwing raw exception types.

nextcloud-android-bot · 2019-12-19T10:27:01Z

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

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 · 2019-12-19T10:36:28Z

Codacy

615

Lint

Type	master	PR
Warnings	73	74
Errors	0	0

SpotBugs (new)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	71
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	106
Security Warnings	44
Dodgy code Warnings	139
Total	413

SpotBugs (master)

Warning Type	Number
Bad practice Warnings	26
Correctness Warnings	70
Internationalization Warnings	13
Malicious code vulnerability Warnings	5
Multithreaded correctness Warnings	9
Performance Warnings	106
Security Warnings	44
Dodgy code Warnings	136
Total	409

6af9523 Merge pull request #4890 from nextcloud/directEditing b4c3a7d Merge pull request #5018 from grote/thumbnail-fix b207671 fix during CI d231370 Merge pull request #4829 from nextcloud/dependabot/gradle/tools.fastlane-screengrab-2.0.0 0d84a2d warning about used feature in newer SDK b4ea014 use master branch ec2cfef Direct editing support - abstract EditorWebView - support direct editing endpoint f2627a2 enable caching ddffaf7 use custom user agent for onlyOffice 0178499 remove unneeded static string f25127d revert to master snapshot 95afe8c use editor id to use any editor d05de98 use file path as parameter for open direct editing file 300abba make sure TextEditor is also only used ond >= Android 5, due to security reason of javascript interface 994817c get direct editing info from endpoint b31cee5 prevent NPE d1611f6 Direct editing support - abstract EditorWebView - support direct editing endpoint 516c464 override minSDK for lib 10a667b Bump screengrab from 1.2.0 to 2.0.0 9fbedb4 Merge pull request #4992 from nextcloud/uiComparison 751301c daily dev 20191219

tobiasKaminsky added the 3. to review label Nov 26, 2019

ezaquarii reviewed Nov 26, 2019

View reviewed changes

AndyScherzinger reviewed Nov 27, 2019

View reviewed changes

src/main/java/com/owncloud/android/ui/activity/TextEditorWebView.kt Outdated

@@ -0,0 +1,38 @@

/*

*

Copy link

Member

AndyScherzinger Nov 27, 2019

Choose a reason for hiding this comment

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

line can e removed

AndyScherzinger reviewed Nov 27, 2019

View reviewed changes

tobiasKaminsky force-pushed the directEditing branch from 5e60212 to a03a6cf Compare December 2, 2019 12:16

tobiasKaminsky mentioned this pull request Dec 4, 2019

Create new text document #4930

Closed

AndyScherzinger approved these changes Dec 9, 2019

View reviewed changes

nextcloud deleted a comment from AndyScherzinger Dec 17, 2019

tobiasKaminsky force-pushed the directEditing branch 2 times, most recently from 2f892e7 to 2900568 Compare December 18, 2019 08:54

tobiasKaminsky added 5 commits December 19, 2019 08:57

Direct editing support

d1611f6

- abstract EditorWebView - support direct editing endpoint Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

prevent NPE

b31cee5

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

get direct editing info from endpoint

994817c

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

make sure TextEditor is also only used ond >= Android 5, due to secur…

300abba

…ity reason of javascript interface Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

use file path as parameter for open direct editing file

d05de98

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

tobiasKaminsky added 8 commits December 19, 2019 08:57

use editor id to use any editor

95afe8c

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

revert to master snapshot

f25127d

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

remove unneeded static string

0178499

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

use custom user agent for onlyOffice

ddffaf7

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

enable caching

f2627a2

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

Direct editing support

ec2cfef

- abstract EditorWebView - support direct editing endpoint Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>

use master branch

b4ea014

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

warning about used feature in newer SDK

0d84a2d

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

tobiasKaminsky force-pushed the directEditing branch from 3a2e573 to 0d84a2d Compare December 19, 2019 07:58

fix during CI

b207671

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

nextcloud-android-bot reviewed Dec 19, 2019

View reviewed changes

src/main/java/com/owncloud/android/ui/fragment/OCFileListFragment.java Show resolved Hide resolved

nextcloud-android-bot reviewed Dec 19, 2019

View reviewed changes

tobiasKaminsky merged commit 6af9523 into master Dec 19, 2019

delete-merged-branch bot deleted the directEditing branch December 19, 2019 14:12

AndyScherzinger added this to the Nextcloud App 3.10.0 milestone Dec 19, 2019

AndyScherzinger mentioned this pull request Dec 19, 2019

Make hyperlinks clickable in markdown previews #5035

Merged

LinneyS mentioned this pull request Jan 16, 2020

Add the correct Content-Disposition to the response when downloading a file ONLYOFFICE/DocumentServer#749

Closed

jakobroehrl mentioned this pull request Jan 22, 2020

Nextcloud Text Editor nextcloud/notes-android#698

Closed

LinneyS mentioned this pull request Jan 22, 2020

User agent clarification ONLYOFFICE/onlyoffice-nextcloud#179

Closed

joshtrichards mentioned this pull request Oct 12, 2023

Add action to a text file in the current directory #417

Closed

-        if (editorWebViewWeakReference.get() instanceof RichDocumentsEditorWebView) {
+        final editorWebViewWeak = editorWebViewWeakReference.get(); // lock reference so it's not GC'd between lines
+        if (editorWebViewWeak == null) {
+            return;
+        }
+        and so on

Uh oh!

Direct editing support #4890

Direct editing support #4890

Uh oh!

Conversation

tobiasKaminsky commented Nov 26, 2019

Uh oh!

ezaquarii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezaquarii Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyScherzinger Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyScherzinger commented Nov 27, 2019

Uh oh!

codecov bot commented Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nextcloud-android-bot commented Dec 5, 2019

Uh oh!

nextcloud-android-bot commented Dec 5, 2019

Uh oh!

AndyScherzinger commented Dec 9, 2019

Uh oh!

elpraga commented Dec 9, 2019

Uh oh!

AndyScherzinger commented Dec 9, 2019

Uh oh!

elpraga commented Dec 9, 2019

Uh oh!

AndyScherzinger commented Dec 9, 2019

Uh oh!

tobiasKaminsky commented Dec 9, 2019

Uh oh!

elpraga commented Dec 9, 2019

Uh oh!

nextcloud-android-bot commented Dec 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezaquarii Nov 26, 2019 •

edited

Loading

AndyScherzinger Nov 27, 2019 •

edited

Loading

codecov bot commented Dec 2, 2019 •

edited

Loading