Gallery of images #277

pablocarmu · 2019-02-19T09:58:17Z

Description

Related Issue

Motivation and Context

How Has This Been Tested?

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)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

BUGS & IMPROVEMENTS

(1) improvement Gallery of images #277 (comment) -> FIXED
(2) Status notification bar Gallery of images #277 (comment) > FIXED
(3) Actions dismiss the gallery Gallery of images #277 (comment)

felix-schwarz · 2019-02-27T21:12:50Z

ownCloud/Gallery/GalleryHostViewController.swift

+		super.init(transitionStyle: .scroll, navigationOrientation: .horizontal, options: nil)
+
+		query.addObserver(self, forKeyPath: "hasChangesAvailable", options: [.initial, .new], context: nil)
+		query.addFilter(itemsFilter, withIdentifier: "items-filter")


As far as I can see looking through the changes on GitHub, you're reusing the OCQuery instance from the ClientQueryViewController instead of creating a new OCQuery, so that adding a filter to only show files would also alter what can be seen in the ClientQueryViewController.

An alternative would be to perform that filtering in observeValue(forKeyPath:…) in the completionHandler of the query.requestChangeSet call.

Also, regarding working KVO: you may need to first merge in the latest changes from ios-app/master to get the latest SDK version which should properly report changes via the hasChangesAvailable property (IIRC all but the latest had cases where it did not).

hosy · 2019-02-28T10:24:05Z

Thank you @pablocarmu for your implementation. Please can you check if the UIViewController has a missing background color? You can see this, if the picture is not downloaded and no network is available:

mneuwert · 2019-02-28T11:28:53Z

ownCloud/Client/ClientQueryViewController.swift

-										if let item = item, item.localID == self?.lastTappedItemLocalID, let core = core {
-											let itemViewController = DisplayHostViewController(for: item, with: core, root: query.rootItem!)
-											self?.navigationController?.pushViewController(itemViewController, animated: true)
+										if let item = item, item.localID == self.lastTappedItemLocalID, let core = core {


@pablocarmu I would probably not mix if let constructs and regular if conditions

I'll split them into 2 if-else

mneuwert · 2019-02-28T11:29:34Z

ownCloud/Gallery/AlternativeCollectionCollectionViewController.swift

+
+private let reuseIdentifier = "Cell"
+
+class AlternativeCollectionCollectionViewController: UICollectionViewController {


@pablocarmu CollectionCollection?

mneuwert · 2019-02-28T11:34:24Z

ownCloud/Viewer/DisplayViewController.swift

-			if let parent = parent, let itemName = item.name {
-				parent.navigationItem.title = itemName
+	func updateNavigationBarItems() {
+		if let parent = parent, let item = item, let itemName = item.name {


@pablocarmu you don't use item but just itemName? Or am I overlooking it?

mneuwert · 2019-02-28T11:38:25Z

ownCloud/Gallery/DisplayViewerViewController.swift

+		if let item = dataSource?.itemFor(viewController: self) {
+			self.item = item
+		} else {
+			if let nonSupportedView = dataSource?.nonSupportedItemView() {


@pablocarmu better notSupported?

ownCloud/Gallery/AlternativeCollectionCollectionViewController.swift

hosy · 2019-03-01T15:09:43Z

ownCloud/Gallery/GalleryHostViewController.swift

+
+	// MARK: - ViewController lifecycle
+    override func viewDidLoad() {
+        super.viewDidLoad()


wrong code indention for the last two lines

hosy · 2019-03-01T15:12:14Z

ownCloud/Gallery/GalleryHostViewController.swift

+				}
+			}
+		}
+    }


also wrong indention

hosy · 2019-03-01T15:15:55Z

ownCloud/Viewer/WebViewDisplayViewController.swift

@@ -130,6 +132,10 @@ extension WebViewDisplayViewController: DisplayExtension {

 extension WebViewDisplayViewController: UIGestureRecognizerDelegate {


wrong indention

michaelstingl · 2019-03-02T12:52:30Z

If I zoom in, then swipe to the next image, then swipe back to the previous one, the previous one is still zoomed in. Bug or feature?

iOS Photos:

Zoom gets reset

iOS Files:

can’t swipe to next when zoomed in (why?)

hosy · 2019-03-03T19:50:15Z

I also think zoom should be reset.

…riendly' - Renamed the galleryhost to be more clear

- Finish the implementation of the gallery using PageViewController. - Code cleanup.

pablocarmu · 2019-03-07T11:33:10Z

Did you profile using Instruments? What takes that much time? Loading or may be decoding of the images?

I didn't make any profile because I've never noticed this smooth-problem, but I'll check it with instruments

mneuwert · 2019-03-07T11:39:28Z

Did you profile using Instruments? What takes that much time? Loading or may be decoding of the images?

I didn't make any profile because I've never noticed this smooth-problem, but I'll check it with instruments

You can also check out https://developer.apple.com/videos/play/wwdc2018/219/
They talk about many useful techniques like downsampling, also you shall keep an eye on memory use.

- Use downsample technique in background to save memory and CPU cycles. - Improve the performance of the gallery.

pablocarmu · 2019-03-07T16:14:36Z

@mneuwert I've used the downsample technique of the talk and I think the performance improvement is great.
Also, I've done it inside a background queue and now the app is not stuck anymore.

mneuwert · 2019-03-07T18:29:23Z

@mneuwert I've used the downsample technique of the talk and I think the performance improvement is great.
Also, I've done it inside a background queue and now the app is not stuck anymore.

@pablocarmu Sounds good! I checked the implementation, and there is one more thing actually. When you zoom in, you will see downsampled and potentially pixelated version of the image, although the source image with higher resolution is available. So may be zooming can be done in more sophisticated way using dynamic downsampling based on the current zoom level, may be combined with tiling techniques in order to avoid memory usage going high. But it is also a question to @michaelstingl if we shall spend that effort or if we treat it as a separate feature.

michaelstingl · 2019-03-07T21:56:25Z

Can be improved in a separate PR… (I hope “Open in” sends the original file, not the downsampled one)

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

codecov · 2019-03-08T11:26:24Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4053c53). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master     #277   +/-   ##
=========================================
  Coverage          ?   29.48%           
=========================================
  Files             ?      229           
  Lines             ?    15477           
  Branches          ?        0           
=========================================
  Hits              ?     4564           
  Misses            ?    10913           
  Partials          ?        0

Impacted Files	Coverage Δ
ownCloud/Viewer/WebViewDisplayViewController.swift	`17.54% <0%> (ø)`
ownCloud/UIKit Extensions/String+Extension.swift	`22.22% <0%> (ø)`
ownCloud/Viewer/DisplayViewController.swift	`0% <0%> (ø)`
ownCloud/Gallery/GalleryHostViewController.swift	`0% <0%> (ø)`
ownCloud/Client/ClientQueryViewController.swift	`34.1% <0%> (ø)`
ownCloud/Viewer/ImageDisplayViewController.swift	`8.54% <0%> (ø)`

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 4053c53...1a6be86. Read the comment docs.

codecov · 2019-03-08T11:26:24Z

Codecov Report

Merging #277 into master will decrease coverage by 0.34%.
The diff coverage is 3.81%.

@@            Coverage Diff             @@
##           master     #277      +/-   ##
==========================================
- Coverage   29.86%   29.52%   -0.35%     
==========================================
  Files         228      229       +1     
  Lines       15284    15484     +200     
==========================================
+ Hits         4564     4571       +7     
- Misses      10720    10913     +193

Impacted Files	Coverage Δ
ownCloud/Viewer/WebViewDisplayViewController.swift	`17.54% <0%> (-1.69%)`	⬇️
ownCloud/UIKit Extensions/String+Extension.swift	`22.22% <0%> (-17.78%)`	⬇️
ownCloud/Viewer/DisplayViewController.swift	`0% <0%> (ø)`	⬆️
ownCloud/Gallery/GalleryHostViewController.swift	`0% <0%> (ø)`
ownCloud/UI Elements/ImageScrollView.swift	`0% <0%> (ø)`	⬆️
ownCloud/Client/ClientQueryViewController.swift	`34.1% <0%> (-0.2%)`	⬇️
ownCloud/Viewer/ImageDisplayViewController.swift	`8.54% <0%> (-4.97%)`	⬇️
ownCloud/Client/ClientRootViewController.swift	`79.29% <100%> (+1.01%)`	⬆️

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 549dcd3...504ab3d. Read the comment docs.

jesmrec · 2019-03-15T11:44:55Z

(1) -> Fixed
(2) -> Fixed
(3) -> will open an issue to improve.

Also, an issue to improve the GIF preview will be opened.

From my side it is OK

mneuwert and others added 4 commits February 12, 2019 21:08

Implemented bookmark display name

83a6c41

Merge branch 'master' into feature/bookmark_displayname

dcb3335

Fixed indentation

627bcd6

Merge branch 'master' into feature/bookmark_displayname

a537c41

pablocarmu added the work in progress label Feb 19, 2019

Implemented different solution to use displayName

789d536

jesmrec assigned pablocarmu Feb 25, 2019

felix-schwarz reviewed Feb 27, 2019

View reviewed changes

pablocarmu force-pushed the feature/gallery_of_images branch from 9239b9d to e846be3 Compare February 28, 2019 09:37

pablocarmu requested a review from jesmrec February 28, 2019 09:43

pablocarmu assigned mneuwert and hosy and unassigned mneuwert and hosy Feb 28, 2019

pablocarmu requested review from mneuwert and hosy February 28, 2019 09:44

jesmrec removed the work in progress label Feb 28, 2019

jesmrec added this to the 1.1.0 milestone Feb 28, 2019

mneuwert suggested changes Feb 28, 2019

View reviewed changes

Merge branch 'master' into feature/bookmark_displayname

fee87b1

hosy requested changes Mar 1, 2019

View reviewed changes

pablocarmu added 6 commits March 4, 2019 08:23

- First gallery approach

5d1ac6b

- Changed the structure of the hostViewController to be more 'query f…

301a6d7

…riendly' - Renamed the galleryhost to be more clear

- Made some improvements in the host controller

4d642b7

- Coded new gesture recognizers

100a2cb

- Removed weak reference of item in DisplayViewController!

ee8df02

- Removed unused code.

bf31d65

- Finish the implementation of the gallery using PageViewController. - Code cleanup.

Merge branch 'master' into feature/gallery_of_images

ea31e59

hosy and others added 2 commits March 7, 2019 12:48

Merge branch 'master' of github.com:owncloud/ios-app

f525a67

- Adapt constants to code style guide.

bbfcc5d

- Use downsample technique in background to save memory and CPU cycles. - Improve the performance of the gallery.

hosy added 2 commits March 8, 2019 11:32

Merge branch 'master' into feature/gallery_of_images

2458f59

# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

Merge branch 'master' into feature/gallery_of_images

1a6be86

pablocarmu added 4 commits March 13, 2019 17:12

- Fixed a nasty bug related with zero bounds inside the image previewer.

a50b263

Merge branch 'master' into feature/gallery_of_images

c5621c0

- Fixed a theme issue in DiplayViewController

3b776d7

- Fixed the progress bar constraint issues

504ab3d

jesmrec approved these changes Mar 15, 2019

View reviewed changes

jesmrec added the Approved by QA Approved by QA label Mar 15, 2019

jesmrec merged commit 868097f into master Mar 15, 2019

delete-merged-branch bot deleted the feature/gallery_of_images branch March 15, 2019 12:04

jesmrec mentioned this pull request Mar 18, 2019

Keep the gallery alive after doing some action over an item #322

Closed

michaelstingl modified the milestones: 1.1.0-Next, 1.0.0-Current Apr 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gallery of images #277

Gallery of images #277

pablocarmu commented Feb 19, 2019 •

edited by jesmrec

Loading

felix-schwarz Feb 27, 2019

hosy commented Feb 28, 2019

mneuwert Feb 28, 2019

pablocarmu Feb 28, 2019

mneuwert Feb 28, 2019

mneuwert Feb 28, 2019

mneuwert Feb 28, 2019

hosy Mar 1, 2019

hosy Mar 1, 2019

hosy Mar 1, 2019

michaelstingl commented Mar 2, 2019

hosy commented Mar 3, 2019

pablocarmu commented Mar 7, 2019

mneuwert commented Mar 7, 2019

pablocarmu commented Mar 7, 2019

mneuwert commented Mar 7, 2019 •

edited

Loading

michaelstingl commented Mar 7, 2019

codecov bot commented Mar 8, 2019

codecov bot commented Mar 8, 2019 •

edited

Loading

jesmrec commented Mar 15, 2019


		private let reuseIdentifier = "Cell"

		class AlternativeCollectionCollectionViewController: UICollectionViewController {

		@@ -130,6 +132,10 @@ extension WebViewDisplayViewController: DisplayExtension {

		extension WebViewDisplayViewController: UIGestureRecognizerDelegate {

Gallery of images #277

Gallery of images #277

Conversation

pablocarmu commented Feb 19, 2019 • edited by jesmrec Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

hosy commented Feb 28, 2019

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

michaelstingl commented Mar 2, 2019

hosy commented Mar 3, 2019

pablocarmu commented Mar 7, 2019

mneuwert commented Mar 7, 2019

pablocarmu commented Mar 7, 2019

mneuwert commented Mar 7, 2019 • edited Loading

michaelstingl commented Mar 7, 2019

codecov bot commented Mar 8, 2019

Codecov Report

codecov bot commented Mar 8, 2019 • edited Loading

Codecov Report

jesmrec commented Mar 15, 2019

pablocarmu commented Feb 19, 2019 •

edited by jesmrec

Loading

mneuwert commented Mar 7, 2019 •

edited

Loading

codecov bot commented Mar 8, 2019 •

edited

Loading