[fix/viewer] Viewer fixes, refactoring and minor improvements #951

felix-schwarz · 2021-04-15T11:55:05Z

Description

fixes various glitches and bugs when displaying files.
refactoring of DisplayViewController and DisplayHostViewController for structural clarity, reinforcement and separation of functionality and concerns
also fixes a few unrelated warnings
asks the user before updating PDF files

Related Issue

#935 #942

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

QA

checks: #951 (comment)

reports:

(1) Loading status: [fix/viewer] Viewer fixes, refactoring and minor improvements #951 (comment)

CLAassistant · 2021-04-15T11:55:11Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ hosy
❌ felix-schwarz
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

…bsystem in one place - DisplayViewController: - use of OCClaim to let the SDK update the item - additional logging around VersionUpdates - consistent tracking of version information and avoidance of re-rendering the same file multiple times - further cleanup - update MDM documentation and doc/CONFIGURATION.json

- factor out generation of displayBarButtonItems - move generation of dots button into actionBarButtonItem method/property - add subclassing point for generation of the standard dots button - PDFViewerViewController: - replace setupToolbar() with PDF-specific implementation based on the previous setupToolbar - add (previously missing?) dots button

- add setup() method as entry point for initializations - add method and property to give viewers control over content refreshes, defaulting to "always update" - DisplayExtension: call setup() method during instance creation - PDFViewController: use new setup() method to change default update strategy for PDF files to "ask"

- DisplayViewController: - unify claim addition / removal in item.didSet - add missing automatic claim removal - further cleanup

hosy · 2021-04-21T09:35:33Z

@felix-schwarz unfortunately I ran into the same problem:
Attached the Xcode logs:
file-could-not-be-opened-log.txt

hosy · 2021-04-21T09:41:20Z

@felix-schwarz what is strange: After running in the problem that the file could not be opened, I did the following:

Tap Manage on the affected Account
Delete Local Copies
Opened the Account
Opened the file ownCloud Manual.pdf and I will get the same error message

I expected that deleting all local copies will download the file again and could be opened.

hosy · 2021-04-21T11:49:47Z

@felix-schwarz I also discovered a crash:

felix-schwarz · 2021-04-21T14:37:01Z

@hosy Thanks for the log. I'll go through it right away.

Regarding the crash, I found no smoking gun in the code at this place (especially the highlighted self.class) or in the lines that call it. Did you compile with Debug or Release?

hosy · 2021-04-21T15:18:02Z

@hosy Thanks for the log. I'll go through it right away.

Regarding the crash, I found no smoking gun in the code at this place (especially the highlighted self.class) or in the lines that call it. Did you compile with Debug or Release?

Debug

- handle case of non-existant local copy of file in DisplayViewController

felix-schwarz · 2021-04-21T22:15:12Z

@hosy Thanks again for the log. It helped track this issue down, which turned out to be a really very specific edge case. I wrote down what happened in a new comment: https://github.com/owncloud/ios-sdk/blob/7abe23141d7a183add451e8770fc38592996f0c5/ownCloudSDK/Core/ItemList/OCCore%2BItemList.m#L570

The issue should finally be fixed now.

ownCloud/Client/Viewer/DisplayViewController.swift

ownCloud/Client/Viewer/README.md

hosy · 2021-04-22T13:59:44Z

@felix-schwarz thanks a lot! Yes, it fixes the issue.
BTW: The refactored code looks great!

Just found a very rare problem:

add the following csv item to dem.owncloud.com:
username.csv
open the item on your device in preview
open the item inside the browser in edit mode
begin to add rows or change values

Mostly the item will be reloaded on the device, but sometimes the preview controller will stuck in "Loading…" state.
Not sure if this is a bug/crash inside the QLPreviewController.

… fix/viewer

felix-schwarz · 2021-04-22T15:24:37Z

@hosy Could reproduce the CSV issue after many tries and found a deadlock that could cause a hang of the main thread + OCCore thread. Fixed the deadlock now. Hope it was the issue you ran into, too.

jesmrec · 2021-04-26T12:31:20Z

QA

#935

New PDF updating feature

Show new version
Refresh without asking
Repeat "Refresh without asking" to assure it woks with more modifications
Ignore updates

Other updates in different kind of files

Update txt -> [fix/viewer] Viewer fixes, refactoring and minor improvements #951 (comment)
Update image

#942 -> not able to reproduce. I tried different ways to check that the file is not opened in preview or markup mode (using a completely new account, browsing through photos, opening different kind of files repeatedly...). Those checks were done with the current develop branch, in commit f466d555

jesmrec · 2021-04-26T12:36:39Z

(1)

Probably is the same issue described above with CSV, but i have reproduced it with a txt file. I did the following steps:

Open a txt file in the device
Open the same file in the browser
Modify the txt file in the browser several times, and check the changes in the app

Finally, the app runs into a Loading... status

I caught some logs: ownCloud_26_Apr_2021_at_14_26_00.log.txt

app: 939956a
sdk: 4483fd5
iPhoneXR v14.4

…iewController for every reload to address (1) in #951

felix-schwarz · 2021-04-27T13:39:03Z

@jesmrec I have been able to reproduce this on the device, but not in the Simulator and implemented a partial fix that creates a new QLPreviewController on every change rather than trying to reload the item.

Partial fix because the issue can still occur, but seems to be internal to QLPreviewController as the file exists, the file contents is also correct, but QLPreviewController simply doesn't finish.

Apparently others have run into this issue before: https://stackoverflow.com/questions/13649555/invalidating-qlpreviewcontroller-cache

I also tried to create a clone of the file with every change and then point QLPreviewController to that file, so it gets new versions under new URLs, so caching is not an issue. However, that didn't succeed either, with QLPreviewController only showing file name + file size information then (Test.txt here, with 4 bytes in it):

I therefore believe that we can't currently work around this issue in QLPreviewController, only decrease its likelihood.

jesmrec · 2021-04-28T10:22:33Z

ok, then the problem under the hood is not totally fixable. We should not forget this, just in case. As i commented, i only reproduce with txt files. #942 is not reproducible on my side as i posted in QA post, we can move this forward and wait for feedback/logs in case someone else can reproduce it. Is this ok for you @hosy ?

hosy · 2021-04-28T10:57:44Z

@jesmrec yes, this works for me. I will also keep this issue in mind and we can move forward. Thanks!

felix-schwarz added 2 commits April 14, 2021 23:14

- Initial cleanup

e001847

Display[Host]ViewController: further cleanup, bugfixes and refactoring

30dd9ca

felix-schwarz linked an issue Apr 15, 2021 that may be closed by this pull request

[BUG] Downloaded file could not be opened #942

Closed

felix-schwarz added this to the 11.6.0-Current milestone Apr 15, 2021

felix-schwarz mentioned this pull request Apr 15, 2021

[BUG] Downloaded file could not be opened #942

Closed

felix-schwarz added 2 commits April 20, 2021 00:14

felix-schwarz linked an issue Apr 20, 2021 that may be closed by this pull request

[BUG] Changed files in Preview not refreshing #935

Closed

felix-schwarz added 2 commits April 20, 2021 17:40

- PDFViewerViewController: fix retain cycle / memory leak

938e80d

- DisplayViewController: - unify claim addition / removal in item.didSet - add missing automatic claim removal - further cleanup

felix-schwarz requested review from hosy and jesmrec April 20, 2021 20:27

- update SDK to fix #942

b4c5a28

- handle case of non-existant local copy of file in DisplayViewController

hosy requested changes Apr 22, 2021

View reviewed changes

ownCloud/Client/Viewer/DisplayViewController.swift Show resolved Hide resolved

ownCloud/Client/Viewer/README.md Show resolved Hide resolved

Add missing localization of "File".

008630a

felix-schwarz requested a review from hosy April 22, 2021 14:01

hosy approved these changes Apr 22, 2021

View reviewed changes

hosy and others added 3 commits April 22, 2021 16:36

Merge branch 'milestone/11.6' into fix/viewer

065fb1d

- update SDK to address deadlock issue

da805b2

Merge branch 'fix/viewer' of https://github.com/owncloud/ios-app into…

939956a

… fix/viewer

felix-schwarz and others added 3 commits April 27, 2021 14:37

PreviewViewController: refactor item rendering to create a new QLPrev…

42e2a64

…iewController for every reload to address (1) in #951

Merge remote-tracking branch 'origin/milestone/11.6' into fix/viewer

6cc1e3d

Configuration documentation updated

a6882a9

jesmrec approved these changes Apr 28, 2021

View reviewed changes

hosy and others added 3 commits April 28, 2021 16:05

Merge branch 'milestone/11.6' into fix/viewer

54616e8

added calens changelog

add7ded

Calens changelog updated

ca32c5d

hosy merged commit f02e5a7 into milestone/11.6 Apr 28, 2021

delete-merged-branch bot deleted the fix/viewer branch April 28, 2021 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[fix/viewer] Viewer fixes, refactoring and minor improvements #951

[fix/viewer] Viewer fixes, refactoring and minor improvements #951

Uh oh!

felix-schwarz commented Apr 15, 2021 •

edited by jesmrec

Loading

Uh oh!

CLAassistant commented Apr 15, 2021 •

edited

Loading

Uh oh!

hosy commented Apr 21, 2021

Uh oh!

hosy commented Apr 21, 2021

Uh oh!

hosy commented Apr 21, 2021

Uh oh!

felix-schwarz commented Apr 21, 2021

Uh oh!

hosy commented Apr 21, 2021

Uh oh!

felix-schwarz commented Apr 21, 2021

Uh oh!

Uh oh!

Uh oh!

hosy commented Apr 22, 2021

Uh oh!

felix-schwarz commented Apr 22, 2021

Uh oh!

jesmrec commented Apr 26, 2021 •

edited

Loading

Uh oh!

jesmrec commented Apr 26, 2021

Uh oh!

felix-schwarz commented Apr 27, 2021

Uh oh!

jesmrec commented Apr 28, 2021 •

edited

Loading

Uh oh!

hosy commented Apr 28, 2021

Uh oh!

Uh oh!

[fix/viewer] Viewer fixes, refactoring and minor improvements #951

[fix/viewer] Viewer fixes, refactoring and minor improvements #951

Uh oh!

Conversation

felix-schwarz commented Apr 15, 2021 • edited by jesmrec Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Screenshots (if appropriate):

Types of changes

QA

Uh oh!

CLAassistant commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hosy commented Apr 21, 2021

Uh oh!

hosy commented Apr 21, 2021

Uh oh!

hosy commented Apr 21, 2021

Uh oh!

felix-schwarz commented Apr 21, 2021

Uh oh!

hosy commented Apr 21, 2021

Uh oh!

felix-schwarz commented Apr 21, 2021

Uh oh!

Uh oh!

Uh oh!

hosy commented Apr 22, 2021

Uh oh!

felix-schwarz commented Apr 22, 2021

Uh oh!

jesmrec commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

QA

Uh oh!

jesmrec commented Apr 26, 2021

(1)

Uh oh!

felix-schwarz commented Apr 27, 2021

Uh oh!

jesmrec commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hosy commented Apr 28, 2021

Uh oh!

Uh oh!

felix-schwarz commented Apr 15, 2021 •

edited by jesmrec

Loading

CLAassistant commented Apr 15, 2021 •

edited

Loading

jesmrec commented Apr 26, 2021 •

edited

Loading

jesmrec commented Apr 28, 2021 •

edited

Loading