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

Feature/thumbnails #32

Merged
merged 4 commits into from
May 18, 2018
Merged

Feature/thumbnails #32

merged 4 commits into from
May 18, 2018

Conversation

felix-schwarz
Copy link
Contributor

@felix-schwarz felix-schwarz commented Apr 28, 2018

Add thumbnail support:

bildschirmfoto 2018-04-28 um 23 01 02

BUGS & IMPROVEMENTS

@felix-schwarz felix-schwarz mentioned this pull request Apr 28, 2018
4 tasks
- Update ios-sdk
- Thumbnail requests are now cancelled if a cell has been moved out of view and may no longer be needed
let displayThumbnail = { (thumbnail: OCItemThumbnail?) in
thumbnail?.requestImage(for: thumbnailSize, scale: 0, withCompletionHandler: { (thumbnail, error, _, image) in
if error == nil {
DispatchQueue.main.async {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you update this branch with what the current master branch has (via rebase) you could get the onMainThread function and wrap the call into:

OnMainThread {
 self.iconView.image = image
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Thanks.

if error == nil {
DispatchQueue.main.async {
if image != nil {
if self.item?.versionIdentifier == thumbnail?.versionIdentifier {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also in Swift you can chain multiple if statements with a ,. Don't know which option is more clear to you if the one with three if statements or chain them like:

if error == nil, image != nil, self.item?.versionIdentifier == thumbnail?.versionIdentifier {
  OnMainThread {
       self.iconView.image = image
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I made that change.

}

if let thumbnail = item.thumbnail {
_ = displayThumbnail(thumbnail)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the displayThumbnail closure is taking the unused thumbnail?.requestImage(...) Bool return as it's own return parameter and this is why it's saying to you Result of call is unused, but produces 'Bool?' when you later call displayThumbnail(thumbnail) if you don't assign it's return value to some variable ( in this case to a _)

You can get rid of this _ assignment if you code it here:

_ = thumbnail?.requestImage(for: thumbnailSize, scale: 0, withCompletionHandler: { (thumbnail, error, _, image)

then you can call the displayThumbnail(thumbnail) without any assignment:

displayThumbnail(thumbnail)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Made that change as well.

@jesmrec
Copy link
Contributor

jesmrec commented May 16, 2018

(1) [FIXED]

  1. Browse into a folder with pics (thumbnails downloaded and displayed)
  2. Browse back to parent folder
  3. Browse again to folder in 1.

Current: Thumbnails not displayed
Expected: Thumbnails displayed

pic1

@jesmrec
Copy link
Contributor

jesmrec commented May 16, 2018

(2) [FIXED]

  1. Open the app with pics in root folder (thumbnails displayed)
  2. Scroll down
  3. Scroll up

Current: Thumbnails in 1. not displayed
Expected: Thumbnails displayed

gif2

@jesmrec
Copy link
Contributor

jesmrec commented May 16, 2018

Formats that thumbnails are not displayed: TIFF, SVG.

@felix-schwarz
Copy link
Contributor Author

The latest commit fixes the issues (1) and (2).

Regarding TIFF and SVG: right now, the app should request thumbnails for all items.

Chances are the server you're testing against doesn't provide thumbnails for this format.

@jesmrec
Copy link
Contributor

jesmrec commented May 18, 2018

All stuff fixed. Approved.

@felix-schwarz felix-schwarz merged commit 8b972dd into master May 18, 2018
@felix-schwarz felix-schwarz deleted the feature/thumbnails branch May 18, 2018 09:12
@jesmrec jesmrec mentioned this pull request Jan 22, 2019
45 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants