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

feat(images): support multi-page jpg reps for tif files #1197

Merged
merged 4 commits into from
Apr 17, 2020

Conversation

ikerr
Copy link
Contributor

@ikerr ikerr commented Apr 14, 2020

No description provided.

@ikerr ikerr requested a review from a team as a code owner April 14, 2020 19:59
jstoffan
jstoffan previously approved these changes Apr 14, 2020
@CLAassistant
Copy link

CLAassistant commented Apr 15, 2020

CLA assistant check
All committers have signed the CLA.

* @inheritdoc
*/
determineViewer(file, disabledViewers = []) {
return this.viewers.find(viewer => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we avoid duplicating the logic from AssetLoader? Can we call super.determineViewer and then further filter the list as needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could probably do something like that if determineViewer() returned a set of viewers rather than a single viewer. Then we could call super.determineViewer() and then further filter the results. Happy to make that change if you'd like, but it'll increase the scope of this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, I could probably create a new method, AssetLoader.determineViewerSet() that returns a set of viewers and then do this:

// AssetLoader
determineViewer(file, disabledViewers = []) {
  const viewers = determineViewerSet(file, disabledViewers);
  if (viewers.length > 0) {
    return viewers[0];
  }

  return undefined;
}

and then in ImageLoader:

// ImageLoader
determineViewer(file, disabledViewers = []) {
  const viewers = super.determineViewerSet(file, disabledViewers);
  // perform further filtering...
}


// If the viewer uses paged assets (e.g., image_2048_jpg), then
// ensure that there is a compatible paged representation.
const pagedRep = (entry.properties || {}).paged === 'true';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm finding the logic here hard to follow. Can we have non-paged TIFF files? What case is this guarding against?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jstoffan I was seeing the following issue... Suppose we have the following reps available:

representations: {                                              
  entries: [                                                  
    {                                                       
      representation: 'jpg',                              
      properties: {                                       
        dimensions: '1024x1024',                        
        paged: 'false',                                 
      },                                                  
    },                                                      
    {                                                       
      representation: 'png',                              
      properties: {                                       
        dimensions: '2048x2048',                        
        paged: 'true',
      },                                                  
    },                                                      
  ],                                                          
}

When AssetLoader.determineViewer(file) is called for a TIFF file, it will see that a "jpg" representation is available (the first entry), and select the following viewer:

{                                                                           
  NAME: 'MultiImage',                                                     
  CONSTRUCTOR: MultiImageViewer,                                          
  REP: 'jpg',                                                             
  EXT: ['tif', 'tiff'],                                                   
  ASSET: '{page}.jpg',                                                    
}

However, the 1024x1024 jpg rep that is available is not a paged rep. So, when we then call ImageLoader.determineRepresentation(), we get back null/undefined because it specifically filters out these reps:

determineRepresentation(file, viewer) {                                     
  return file.representations.entries.find(entry => {                     
    // Do not use the dimensions=1024x1024&paged=false rep that is for document preloading
    if (entry.properties && entry.properties.paged === 'false') {       
      return false;                                                   
    }                                                                   
                                                                                
    return viewer.REP === entry.representation;                         
   });                                                                     
}

This most recent change aims to only use the MultiImage viewer (which is currently only used for TIFF files) when a paged representation exists.

Copy link
Collaborator

@jstoffan jstoffan left a comment

Choose a reason for hiding this comment

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

Thank you!

@jstoffan jstoffan merged commit 206350e into box:master Apr 17, 2020
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