littleredbutton / bigbluebutton-api-php

Unofficial (but better) PHP API for @BigBlueButton
GNU Lesser General Public License v3.0
25 stars 12 forks source link

Expose all formats of a recording #109

Closed sualko closed 2 years ago

sualko commented 2 years ago

This pr provides a way to access all available playback formats of a recording. To not break anything, I keep the existing API as a shortcut to quickly access the first format. Does someone have an idea how this could be documented? Is it enough to write it into the method description?

Another thing is that all previews are converted to an array which is maybe not the most elegant solution. What do you think? Should this also wrapped into a class?

fix #66

SamuelWei commented 2 years ago

This pr provides a way to access all available playback formats of a recording. To not break anything, I keep the existing API as a shortcut to quickly access the first format. Does someone have an idea how this could be documented? Is it enough to write it into the method description?

What about deprecating the getters of the url and length and removing in the next major release?

Another thing is that all previews are converted to an array which is maybe not the most elegant solution. What do you think? Should this also wrapped into a class?

We could lazy load the preview images, by only creating the array if the getImagePreviews method is called and cache the result for future calls.

Update: We could also lazy load the playback formats in the getPlaybackFormats the same way.

Pseudo code, not tested. Update: Tests would work fine. I did not benchmark or test with real data.

    /** @var \SimpleXMLElement */
    private $previewImagesRaw;

    /** @var array */
    private $imagePreviews;

    public function __construct(\SimpleXMLElement $xml)
    {
        ...
        $this->previewImagesRaw = $xml->preview->images;
    }

    public function hasImagePreviews(): bool
    {
        return return (bool) $this->previewImagesRaw;
    }

    public function getImagePreviews(): array
    {
        if ($this->imagePreviews!=null) {

            return $this->imagePreviews;

        }

        $this->imagePreviews = [];
        if ($this->previewImagesRaw) {
            foreach ($this->previewImagesRaw->children() as $image) {
                $attributes = $image->attributes();

                $this->imagePreviews[] = [
                'width'  => (int) $attributes->width->__toString(),
                'height' => (int) $attributes->height->__toString(),
                'alt'    => $attributes->alt->__toString(),
                'url'    => $image->__toString(),
            ];
            }
        }
        return $this->imagePreviews;
    }
sualko commented 2 years ago

What about deprecating the getters of the url and length and removing in the next major release?

That would be another possibility. Maybe the easiest thing to do.

We could lazy load the preview images, by only creating the array if the getImagePreviews method is called

I think this is a good enhancement and I will add asap, but my question was more if we should use a wrapper similar to the PlaybackFormat class. In my opinion it's maybe a bit too much, but I'm not sure.

SamuelWei commented 2 years ago

I think this is a good enhancement and I will add asap, but my question was more if we should use a wrapper similar to the PlaybackFormat class. In my opinion it's maybe a bit too much, but I'm not sure.

Have a look at #110. Just a quick idea how to use the same technique for other properties like playbackFormats

sualko commented 2 years ago

If we look at other response classes like CreateMeetingResponse the properties are not cached and maybe therefore we should do the same in PlaybackFormat.

sualko commented 2 years ago

If this gets merged, I think we are ready for the release of 4.2. @SamuelWei thanks for preparing the release.