Skip to content

Conversation

@pcambra
Copy link
Contributor

@pcambra pcambra commented Feb 25, 2021

This PR removes the container class from the iiif div so the width is always 100%.
Also some minor coding style fixes

Fixes #125

@pcambra pcambra requested a review from DiegoPino February 25, 2021 18:40
Copy link
Member

@DiegoPino DiegoPino left a comment

Choose a reason for hiding this comment

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

Awesome @pcambra. single css fix 👏 but with the DCS it touches a lot of code so took me a bit longer to review.
There is one line you removed that needs to go back but also needs to be fixed (and issue on my part) and tested (by clearing caches and starting new annotations, the changing user, etc) the code way this works is also good to explore for you. Thanks a lot

Also, about the file cache. We normally do not remove @TODO except if we are over them or we get them fixed or they are not relevant. In this case I do not know enough to be sure if the file cache should be taken in account here or not. If you feel its not needed all good and we are over this, but if not, either leave the comment there or if you feel so add them a cache tag(s) for the actual render array.

// This also never runs if cached. So after deletion we better
// call the controller!
if (!empty($jsondata['ap:annotationCollection']) && is_array($jsondata['ap:annotationCollection'])) {
$keystoreid = $elements[$delta]['media' . $i]['#attached']['drupalSettings']['format_strawberryfield']['openseadragon'][$uniqueid]['keystoreid'];
Copy link
Member

Choose a reason for hiding this comment

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

This here seems to be a bug/of mine
WebAnnotationController::primeKeyStore() should be primed with the $keystoreid. Issue was this whole thing is cached and gets actually never call again after a cache clear. So wondering if it can be removed totally OR, better not and we need to prime it, at least once to avoid problems when the cache expires (e.g because of Node edit).

Can you please change to to WebAnnotationController::primeKeyStore($keystoreid) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Almost! Sorry again (what a day!) its not $elements[$delta] its $items[$delta], $elements[$delta] its an render array, $items[$delta] the actual field with JSON data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops sorry about that!

public function viewElements(FieldItemListInterface $items, $langcode) {
$elements = [];
$max_width = $this->getSetting('max_width');
$max_width_css = empty($max_width) || $max_width == 0 ? '100%' : $max_width .'px';
Copy link
Member

Choose a reason for hiding this comment

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

yep. what a shame! sloppy iterative code. Thanks!

// @see https://github.com/esmero/format_strawberry/issues/1

//@ TODO recheck cache tags here, since we are not really using the file itself.
$filecachetags = $file->getCacheTags();
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if we should add the file cache tags instead of removing this? (Given its and not Solved @TODO) or do you think it is not 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.

Aw sorry about that, we're not doing anything with the tags so maybe some more work is needed for this?

Copy link
Member

Choose a reason for hiding this comment

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

No problem. Let's keep the //@todo for now and remove 240. We can work on that in another issue. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Hey. I will merge this. We can deal with the @todo another day. I have done the cache stuff on the WARC formatter so we can copy from there. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the cache tags to the element delta, is that what would be intended?

'#default_value' => $uniqueid,
'#attributes' => [
'id' => $uniqueid,
'class' => ['strawberry-media-item','field-iiif','container'],
Copy link
Member

Choose a reason for hiding this comment

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

And this the actual fix! Good. Good.

@DiegoPino
Copy link
Member

Still need to wait for tomorrow to merge because of the non initialized (my fault) storage. Let's get that done and we move from there. I will merge the Twig editor in the meantime!

if (!empty($jsondata['ap:annotationCollection']) && is_array($jsondata['ap:annotationCollection'])) {
WebAnnotationController::primeKeyStore($items[$delta]);
$keystoreid = $elements[$delta]['media' . $i]['#attached']['drupalSettings']['format_strawberryfield']['openseadragon'][$uniqueid]['keystoreid'];
WebAnnotationController::primeKeyStore($keystoreid);
Copy link
Member

Choose a reason for hiding this comment

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

Hi Pedro, sorry again. I explained myself wrong here: It is both, look at the primeKeyStore signature.

/**
   * Primes the Web Annotation KeyStore with saved values.
   *
   * @param \Drupal\strawberryfield\Plugin\Field\FieldType\StrawberryFieldItem $itemfield
   * @param null $keystoreid
   */
  public static function primeKeyStore(StrawberryFieldItem $itemfield,  $keystoreid = NULL) {
    if ($keystoreid == NULL && strlen(trim($keystoreid)) == 0) {
      return NULL;
    }
    $jsondata = $itemfield->provideDecoded(TRUE);
    $tempstore = \Drupal::service('tempstore.private')->get(
      'webannotation'
    );
    if (!empty($jsondata['ap:annotationCollection']) && is_array($jsondata['ap:annotationCollection'])) {
      $tempstore->set($keystoreid, $jsondata['ap:annotationCollection']);
      return $jsondata['ap:annotationCollection'];
    }
    else {
      $tempstore->set($keystoreid, []);
      return [];
    }
  }
``` this won't work that way. You can look at how we call this in other parts.

The issue I had was that I was passing only the $items[$delta] when we already know where we want the keystore to be primed. But also this needs some little testing if you could. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see that now, not sure why PHPStorm didn't call me out when I did that

@pcambra pcambra force-pushed the ISSUE-125/openseadragon branch from 4a4518c to 65a3041 Compare February 26, 2021 14:33
@pcambra pcambra force-pushed the ISSUE-125/openseadragon branch from 65a3041 to ff84c1a Compare February 26, 2021 15:31
@DiegoPino DiegoPino changed the base branch from main to 1.0.0-RC2 March 2, 2021 21:19
@DiegoPino DiegoPino merged commit 9f0241d into 1.0.0-RC2 Mar 2, 2021
@DiegoPino
Copy link
Member

And merging. Thanks @pcambra !

@DiegoPino DiegoPino deleted the ISSUE-125/openseadragon branch July 24, 2025 19:40
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.

Full screen view cut in half w/ OpenSeadragon viewer

3 participants