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

FeedFetcher: Use feeds logo instead of the favicon #1164

Merged
merged 1 commit into from
Feb 16, 2021
Merged

FeedFetcher: Use feeds logo instead of the favicon #1164

merged 1 commit into from
Feb 16, 2021

Conversation

Grotax
Copy link
Member

@Grotax Grotax commented Feb 11, 2021

This changes the behavior of fetching the favicon, since feed-io offers to get the feeds logo, we try to get that first, then fall back to the favicon.

I didn't change the var name, I'm not sure if we should do that. But that would include API changes if I'm not wrong.

Also I want to test this first.

@Grotax Grotax added enhancement API Impact API/Backend code labels Feb 11, 2021
@SMillerDev
Copy link
Contributor

I think this should also have a unittest associated with it.

@anoymouserver
Copy link
Contributor

anoymouserver commented Feb 11, 2021

Not sure if that's good idea to use it as the favicon in front of a feed. According to the Atom standard the logo has the aspect ratio of 2:1 and the RSS standard seems to also prefer a non-square image.

@Grotax
Copy link
Member Author

Grotax commented Feb 11, 2021

@anoymouserver I doubt that anyone adheres to it, the only feed I found didn't it was simply the wordpress 32x32 favicon
Also I think clients should be able to handle it if it happens

@Grotax
Copy link
Member Author

Grotax commented Feb 11, 2021

I would also love to increase the size of the favicons we can pull, but thats not supported by the lib

@anoymouserver
Copy link
Contributor

I would also love to increase the size of the favicons we can pull, but thats not supported by the lib

Wouldn't it be better then to extend the 3rd-party lib to prefer higher resolutions (or even better SVGs) (ArthurHoaro/favicon#12)?

@Grotax
Copy link
Member Author

Grotax commented Feb 11, 2021

I would also love to increase the size of the favicons we can pull, but thats not supported by the lib

Wouldn't it be better then to extend the 3rd-party lib to prefer higher resolutions (or even better SVGs) (ArthurHoaro/favicon#12)?

I think I would prefer that, as it would bring an improvement to almost all feeds.
But I also think that it's not easy, the only standard I know is the one apple introduced at some point.

@anoymouserver
Copy link
Contributor

Could we check if the feed's logo is square and if not, just use the favicon?
I'll eventually have a look if we can query bigger sizes or even better SVG favicons.

@Grotax
Copy link
Member Author

Grotax commented Feb 13, 2021

@anoymouserver sounds good I will check how to do that.

@Grotax
Copy link
Member Author

Grotax commented Feb 14, 2021

Do we want to check the aspect ratio every time we build the feed, like this?

If yes I will try to fix the unit tests

@Grotax
Copy link
Member Author

Grotax commented Feb 15, 2021

I will manually squash the commits before merging

Copy link
Contributor

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

Just some nitpicks. Feature looks good.

lib/Fetcher/FeedFetcher.php Outdated Show resolved Hide resolved
lib/Fetcher/FeedFetcher.php Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Feb 16, 2021

Codecov Report

Merging #1164 (f62a871) into master (961c561) will decrease coverage by 0.38%.
The diff coverage is 54.54%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1164      +/-   ##
============================================
- Coverage     91.34%   90.96%   -0.39%     
- Complexity      662      665       +3     
============================================
  Files            57       57              
  Lines          2358     2369      +11     
============================================
+ Hits           2154     2155       +1     
- Misses          204      214      +10     
Impacted Files Coverage Δ Complexity Δ
lib/Fetcher/Fetcher.php 100.00% <ø> (ø) 5.00 <0.00> (ø)
lib/Service/FeedServiceV2.php 100.00% <ø> (ø) 28.00 <0.00> (ø)
lib/Fetcher/FeedFetcher.php 88.23% <52.38%> (-6.89%) 36.00 <7.00> (+3.00) ⬇️
lib/Command/ShowFeed.php 100.00% <100.00%> (ø) 4.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 961c561...f62a871. Read the comment docs.

The logo of the feed is prefered if it is a square picture,
else the favicon is returned.

Signed-off-by: Benjamin Brahmer <[email protected]>
Co-authored-by: Sean Molenaar <[email protected]>
@Grotax Grotax merged commit c09b4d8 into master Feb 16, 2021
@delete-merged-branch delete-merged-branch bot deleted the use_logo branch February 16, 2021 09:16
Grotax added a commit that referenced this pull request Feb 23, 2021
Changed
- Remove outdated item DB code. ( #1056)
- Stop returning all feeds after marking folder as read. (#1056)
- Always fetch favicon (#1164)
- Use feed logo instead of favicon if it exists and is square (#1164)
- Add CI for item lists (#1180)

Fixed
- Item list throwing error for folder and "all items" (#1180)
- Articles with high IDs can be placed lower than articles with low IDs (#1147)
- Feeds are accidentally moved on rename (#1189)
- Item list not using ID for offset (#1188)

Signed-off-by: Benjamin Brahmer <[email protected]>
@Grotax Grotax mentioned this pull request Feb 23, 2021
Grotax added a commit that referenced this pull request Feb 23, 2021
Changed
- Remove outdated item DB code. ( #1056)
- Stop returning all feeds after marking folder as read. (#1056)
- Always fetch favicon (#1164)
- Use feed logo instead of favicon if it exists and is square (#1164)
- Add CI for item lists (#1180)

Fixed
- Item list throwing error for folder and "all items" (#1180)
- Articles with high IDs can be placed lower than articles with low IDs (#1147)
- Feeds are accidentally moved on rename (#1189)
- Item list not using ID for offset (#1188)

Signed-off-by: Benjamin Brahmer <[email protected]>
mnassabain pushed a commit to Team-Forward/news that referenced this pull request Mar 1, 2021
Changed
- Remove outdated item DB code. ( nextcloud#1056)
- Stop returning all feeds after marking folder as read. (nextcloud#1056)
- Always fetch favicon (nextcloud#1164)
- Use feed logo instead of favicon if it exists and is square (nextcloud#1164)
- Add CI for item lists (nextcloud#1180)

Fixed
- Item list throwing error for folder and "all items" (nextcloud#1180)
- Articles with high IDs can be placed lower than articles with low IDs (nextcloud#1147)
- Feeds are accidentally moved on rename (nextcloud#1189)
- Item list not using ID for offset (nextcloud#1188)

Signed-off-by: Benjamin Brahmer <[email protected]>
Signed-off-by: Marco Nassabain <[email protected]>
mnassabain pushed a commit to Team-Forward/news that referenced this pull request Mar 1, 2021
Changed
- Remove outdated item DB code. ( nextcloud#1056)
- Stop returning all feeds after marking folder as read. (nextcloud#1056)
- Always fetch favicon (nextcloud#1164)
- Use feed logo instead of favicon if it exists and is square (nextcloud#1164)
- Add CI for item lists (nextcloud#1180)

Fixed
- Item list throwing error for folder and "all items" (nextcloud#1180)
- Articles with high IDs can be placed lower than articles with low IDs (nextcloud#1147)
- Feeds are accidentally moved on rename (nextcloud#1189)
- Item list not using ID for offset (nextcloud#1188)

Signed-off-by: Benjamin Brahmer <[email protected]>
Signed-off-by: Marco Nassabain <[email protected]>
mnassabain pushed a commit to Team-Forward/news that referenced this pull request Mar 1, 2021
Changed
- Remove outdated item DB code. ( nextcloud#1056)
- Stop returning all feeds after marking folder as read. (nextcloud#1056)
- Always fetch favicon (nextcloud#1164)
- Use feed logo instead of favicon if it exists and is square (nextcloud#1164)
- Add CI for item lists (nextcloud#1180)

Fixed
- Item list throwing error for folder and "all items" (nextcloud#1180)
- Articles with high IDs can be placed lower than articles with low IDs (nextcloud#1147)
- Feeds are accidentally moved on rename (nextcloud#1189)
- Item list not using ID for offset (nextcloud#1188)

Signed-off-by: Benjamin Brahmer <[email protected]>
Signed-off-by: Marco Nassabain <[email protected]>
Neo11 pushed a commit to Neo11/news that referenced this pull request May 28, 2022
Changed
- Remove outdated item DB code. ( nextcloud#1056)
- Stop returning all feeds after marking folder as read. (nextcloud#1056)
- Always fetch favicon (nextcloud#1164)
- Use feed logo instead of favicon if it exists and is square (nextcloud#1164)
- Add CI for item lists (nextcloud#1180)

Fixed
- Item list throwing error for folder and "all items" (nextcloud#1180)
- Articles with high IDs can be placed lower than articles with low IDs (nextcloud#1147)
- Feeds are accidentally moved on rename (nextcloud#1189)
- Item list not using ID for offset (nextcloud#1188)

Signed-off-by: Benjamin Brahmer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review API Impact API/Backend code enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants