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

Add four (down from 5) new strips, fix one old one #108

Merged

Conversation

ExTechOp
Copy link

@ExTechOp ExTechOp commented Nov 5, 2024

Added five new strips (with hopefully accurate information and suitable cover + sample images):

  • bizarro (Comicskingdom)
  • haraldhirmuinen (Helsingin Sanomat)
  • keskenkasvuisia (Helsingin Sanomat)
  • lassijaleevi (Helsingin Sanomat)
  • thefarside (Larson's website thefarside.com)

And corrected retrieval of kamalaluonto (now from Keskisuomalainen newspaper), and updated also its information.

Closes issues #103 and #108.

@ExTechOp
Copy link
Author

ExTechOp commented Nov 5, 2024

And only after submitting this I realized that thefarside does properly retrieve the images, but they do not include the captions, so most of them are rather incomprehensible 😿

@Olf0
Copy link

Olf0 commented Nov 5, 2024

Thank you very much! Now I feel a bit obliged to create a new release, soon (which is not a bad thing, see below). I hope I can spare the time the next weekend.

WRT "The Far Side": I am a fan of it since being on school exchange in Washington state in the late 1980ies. Updating DailyComic's metadata for it a few years ago let me finally understand why my "exchange mother" was such a big fan (and infected me): Gary Larson is from Seattle. But please see PR #88! Edit: You did better than I did, but without the captions the comic strips make no sense, unfortunately. Hence the sad consequence is the same. Side note: BTW, #ffffff (all white) is a "illegal" background colour.
Thus I will delete it from this PR (before merging).

I will try to quickly review the rest of this PR in the next days.

P.S.: Your contributions mean a big uplift for my motivation to maintain DailyComics. Because the original creator did stall all PR for years (without deciding to merge them or not), I closed all my PRs on 2023-09-23 and forked DailyComics (in the original sense of "to fork", i.e. "to continue this project separately"), after unsuccessfully proposing to collaborate a few times.

P.P.S.: You can use GitHub's search (input field in top left of its web-frontend page) to avoid redoing things which were tried before. E.g. searching for "foo" in this repository results in this search link:
https://github.com/search?q=repo%3Asailfishos-applications%2Fdaily-comics+foo&type=code
Then you can click through the different categories in the side bar at the left (aforementioned link displays the category code first), which also indicates the number of search-hits in each category.

@ExTechOp
Copy link
Author

ExTechOp commented Nov 6, 2024

Yes, I concur that The Far Side like this doesn't really work and including it like this doesn't make sense. One future enhancement might be to be able to display text alongside the image — that would also be the place to include the xkcd rollover texts.

I will admit I still don't quite understand what use the program makes of the specified color, I cannot recall having seen any comics or cover images on my phone on anything but a black background? The colors I've specified with these patches were mostly taken from the cover image, assuming this somehow is representative of color scheme. Edit: for example, what should it be for black-and-white comics?

@Olf0
Copy link

Olf0 commented Nov 7, 2024

I concur that The Far Side like this doesn't really work and including it like this doesn't make sense.

Deleted by commit bce1dce.

One future enhancement might be to be able to display text alongside the image — that would also be the place to include the xkcd rollover texts.

If you want to avoid this to become forgotten, please file a feature request as a new issue suggesting exactly this (maybe a little bit more fleshed out).
It might be possible to render such text to JPEG format and fuse the two images into one (the new image with the rendered text seamlessly below the original image) in JavaScript, but I have no specific idea without a lot of research, because I am no JavaScript, QML, C++, Python etc. programmer (I can hardly understand and rectify simple code in these languages).

I will admit I still don't quite understand what use the program makes of the specified color, I cannot recall having seen any comics or cover images on my phone on anything but a black background?

When I reworked the README.md and all other guidance (e.g. info/comic_addition.md) almost a year ago, I tried to retrace all requirements @tardypad originally posed: Is it a hard or soft requirement (i.e. is it appropriate to use "must" / "shall" or rather "should" for this requirement), does this requirement make sense at all etc. While I do not remember the specifics of the requirement "background colour must not be all white (#ffffff) or all black (#00000)", I think (i.e. hope) I did that with my usual diligence: I.e. it once made sense to me to keep this requirement in the documentation, IIRC.

The colors I've specified with these patches were mostly taken from the cover image, assuming this somehow is representative of color scheme.

Correct, this is another (but rather soft) requirement stated in info/comic_addition.md (which absolutely makes sense IMO).

Edit: for example, what should it be for black-and-white comics?

Pick a different, but similar background colour. 😉 E.g. "almost white" (e.g. #f0f0f0) / "almost black" (i.e. "dark grey", e.g. #0f0f0f).
Or (better) look for a "true, colourful colour" regularly appearing in that comic strip, its logo, main character(s) etc. (i.e. besides black and / or white, or any shade of grey), and specify that as background colour.

@Olf0 Olf0 self-assigned this Nov 10, 2024
@Olf0 Olf0 mentioned this pull request Nov 16, 2024
@Olf0
Copy link

Olf0 commented Dec 2, 2024

My four review comments as screenshots, because something is apparently not working with GitHub's "review"-functionality, lately.

As already stated, only the first two contain real questions, the other two are just comments (feel free to comment them, too).

  1. A single question
    screenshot_2024-12-02_020144
  2. Two questions
    screenshot_2024-12-02_020435
  3. Just a comment
    screenshot_2024-12-02_020546
  4. Another comment
    screenshot_2024-12-02_020619

@ExTechOp
Copy link
Author

ExTechOp commented Dec 4, 2024

  1. Yep, I can fully understand why you prefer that. Go ahead with the change.
  2. Also this Finnish language Wikipedia change is fine.
  3. I cannot claim to be an expert of javascript, but the "object-oriented" syntax of calling the String method seems most natural here. Replace replaces the matched regex here in the URL with just the string "FREE" without a trailing number, which seems to provide the highest-resolution image.

@Olf0
Copy link

Olf0 commented Dec 5, 2024

Done.

Done.

  1. I cannot claim to be an expert of javascript, but the "object-oriented" syntax of calling the String method seems most natural here.

That is the point: C, Shell-scripts, Fortran, Assembler, Modula, Pascal, Basic are all fine for me, but OO means "Ooooh" to me instead of "Object Oriented", i.e. I have (almost) no clue.

…, which seems to provide the highest-resolution image.

Perfect, as you checked and implemented that; hence it is fine for me.

@Olf0 Olf0 merged commit 9829968 into sailfishos-applications:devel Dec 5, 2024
1 check passed
Copy link

@Olf0 Olf0 left a comment

Choose a reason for hiding this comment

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

Fine now.

plugins/keskenkasvuisia/info.json Outdated Show resolved Hide resolved
plugins/lassijaleevi/info.json Outdated Show resolved Hide resolved
plugins/kamalaluonto/extract.js Show resolved Hide resolved
plugins/haraldhirmuinen/info.json Show resolved Hide resolved
Olf0 added a commit to ExTechOp/daily-comics that referenced this pull request Dec 5, 2024
Already did that in PR sailfishos-applications#108, for reasons see there.
@Olf0 Olf0 changed the title Add 5 new strips, fix one old one Add ~~5~~ 4 new strips, fix one old one Dec 7, 2024
@Olf0 Olf0 changed the title Add ~~5~~ 4 new strips, fix one old one Add 4 (down from 5) new strips, fix one old one Dec 7, 2024
@Olf0 Olf0 changed the title Add 4 (down from 5) new strips, fix one old one Add four (down from 5) new strips, fix one old one Dec 7, 2024
@ExTechOp ExTechOp deleted the helsingin_sanomat_and_more branch December 9, 2024 10:08
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.

2 participants