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

Basic Media-RSS support #599

Merged
merged 1 commit into from
Feb 16, 2020
Merged

Basic Media-RSS support #599

merged 1 commit into from
Feb 16, 2020

Conversation

azmeuk
Copy link
Contributor

@azmeuk azmeuk commented Dec 28, 2019

This is an attempt to tackle #570

This adds support for media thumbnails and description, available since Feedio 4.5.

Now youtube feeds display correctly:
Screenshot_2019-12-28 News (13) - Nextcloud

I am not a PHP expert, neither an angular one, so I am not really used to good practices with those tools. Do not hesitate to tell me if there is anything I can improve.

Signed-off-by: Éloi Rivard <azmeuk@gmail.com>
@SMillerDev
Copy link
Contributor

I'm not entirely sure if this should be a new element. Wouldn't you be able to represent the new images and description in the current datamodel?

@azmeuk
Copy link
Contributor Author

azmeuk commented Dec 29, 2019

Wouldn't you be able to represent the new images and description in the current datamodel?

Do you mean for example directly add some HTML into the feed content to avoid store the thumbnail and the description in new elements?

The current data model has those elements:

    /** @var string|null */
    protected $contentHash;
    /** @var string */
    protected $guidHash;
    /** @var string */
    protected $guid;
    /** @var string|null */
    protected $url;
    /** @var string|null */
    protected $title;
    /** @var string|null */
    protected $author;
    /** @var int|null */
    protected $pubDate;
    /** @var int|null */
    protected $updatedDate;
    /** @var string|null */
    protected $body;
    /** @var string|null */
    protected $enclosureMime;
    /** @var string|null */
    protected $enclosureLink;
    /** @var int */
    protected $feedId;
    /** @var int */
    protected $status = 0;
    /** @var string|null */
    protected $lastModified = '0';
    /** @var string|null */
    protected $searchIndex;
    /** @var bool */
    protected $rtl = false;
    /** @var string|null */
    protected $fingerprint;
    /** @var bool */
    protected $unread = false;
    /** @var bool */
    protected $starred = false;

I do not see pertinent attributes to store the thumbnail. We could store the description in $body, but according to the Media-RSS specification, the description of a media element is not the same thing as the feed content (even if for youtube feeds, it actually is). Do you have an idea to improve the implementation I proposed?

I understand that in fact the whole media model should be reworked, since a feed item can host several medias, but the current model only allows to manage one.

@SMillerDev
Copy link
Contributor

Maybe you can add a description field only and make the thumbnail the media?

@azmeuk
Copy link
Contributor Author

azmeuk commented Dec 29, 2019

Technically that would work, but it seems to me that it would not be a good model representation of what a media is. The MediaRSS specification details a lot of attributes for a media object, including a thumbnail. Youtube and Peertube feeds for example provides both a media URL and a thumbnail URL. I suppose a good way for nextcloud-news to take benefit of this would be to display the thumbnail when the player is not available.

@Grotax Grotax self-requested a review January 1, 2020 12:32
@stale
Copy link

stale bot commented Jan 22, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Jan 22, 2020
@azmeuk
Copy link
Contributor Author

azmeuk commented Jan 22, 2020

Please stalebot let this issue live.

@stale stale bot removed the stale label Jan 22, 2020
@Grotax
Copy link
Member

Grotax commented Jan 27, 2020

Didn't have the time to review this, yet 😅

Copy link
Member

@Grotax Grotax left a comment

Choose a reason for hiding this comment

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

I think this is fine, @SMillerDev what do you think?

@Grotax Grotax merged commit 306d3cd into nextcloud:master Feb 16, 2020
@Grotax
Copy link
Member

Grotax commented Feb 16, 2020

Thanks @azmeuk sorry for the delay.

@Grotax Grotax mentioned this pull request Mar 12, 2020
Grotax added a commit that referenced this pull request Mar 18, 2020
Changed
- Basic Media-RSS support (#599)
- Database index improvements (#637)

Fixed
- Call to a member function getUrlHash() on null" when adding a feed (#640)
- Don't install symfony/console via composer (#636)
- Fix for for ONLY_FULL_GROUP_BY (see #406) (Issue #80) (#407)
- Catch invalid feeds (#646)

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@user858753257
Copy link

I have added a youtube feed to my list. But the content view is empty. So now video preview....

@azmeuk
Copy link
Contributor Author

azmeuk commented Apr 27, 2020

What is the link of your Youtube feed? I works like a charm for mines.

@user858753257
Copy link

What is the link of your Youtube feed? I works like a charm for mines.

For testing purpose i have used this

https://www.youtube.com/channel/UCbiGcwDWZjz05njNPrJU7jA

@Grotax
Copy link
Member

Grotax commented May 11, 2020

I tested your url and it works fine.
Bildschirmfoto_2020-05-11_19-14-47

@user858753257
Copy link

I think the latest update have resolved my issue .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants