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

[plugins/fingerpori] Update extract.js (superseded by #105) #104

Closed
wants to merge 1 commit into from

Conversation

ExTechOp
Copy link

@ExTechOp ExTechOp commented Oct 30, 2024

This is a prototype fix for issue #103.

Simplified the regex as much as I could (the issue was that it included the name of the media delivery host, and it had changed) and included prepending "https:" method to the string.
@ExTechOp
Copy link
Author

ExTechOp commented Oct 30, 2024

Simplified the regex as much as I could (the issue was that it included the name of the media delivery host, and it had changed) and included prepending "https:" method to the string.

This is my POC of the proposed change to all the HS strips which resolves #103 for Fingerpori — if this is acceptable, I'll make the same change to all of them.

@Olf0 Olf0 changed the title Update extract.js [Fingerpori comic] Update extract.js Oct 31, 2024
@Olf0
Copy link

Olf0 commented Oct 31, 2024

Thank you very much. The changes to Fingerpori's extract.js are looking reasonable to me.

Two checks & balances questions:

  1. Have you employed these changes locally on your SailfishOS device (in /usr/share/harbour-dailycomics/plugins/fingerpori/) and is it working (i.e. extracting the image) fine?
  2. Have you checked if the Metadata is correct and up-to-date?
  3. Did the RegEx I suggested not work?
    Background: IIRC I had the impression that the data-srcset provides a higher resolution image.

P.S.: I currently lack the time to review PRs as thoroughly as I would like (and feel I should), hence I am asking.

P.P.S.: I moved your first comment to the start of the second, because GitHub uses the first comment as commit details when merging a PR (one can change that then, but I doubt that I still remember to do that then).

@Olf0 Olf0 changed the title [Fingerpori comic] Update extract.js [plugins/fingerpori] Update extract.js Oct 31, 2024
@ExTechOp
Copy link
Author

ExTechOp commented Oct 31, 2024

  1. Yes, though I used the rather crude method of directly editing /usr/share/harbour-dailycomics/plugins/fingerpori/extract.js of a running installation, as I too lack the time to become a proper Sailfish developer (and a Github expert)
  2. Didn't really think about that part at the time, but looking at it now it everything still seems to be valid
  3. At least currently this ends up matching exactly the same part (and retrieving the 1920 px version); I wanted a minimal pattern to avoid dependencies on things that might change, as previously happened

How do we proceed from here, do you want to merge this in, or shall I create a whole new fork which updates all the HS strips with this same method?

@Olf0
Copy link

Olf0 commented Nov 2, 2024

  1. It is exactly this "rather crude method" I was thinking of. 😄 👍
    Unix allows for "crude" ways to work properly, if you know what you are doing; this is one of the reasons I like it.

  2. Thank you!

  3. But them my original suggestion would be better (if it is working fine) according to your criterion, because it is shorter.
    It ultimately does not really matter, both variants are fine.

  4. As you like it. I see two schemes which make sense to me:

    • Fix all extract.js files of all extant HS-comics plug-ins in this PR (and hence in your branch patch-1 of your git-clone / "fork" of this git-repository).
    • Fix the extant HS-comics plug-ins one by one (extract.js and metadata), each in a separate PR.
      As you already have checked the metadata for Fingerpori this PR is good to merged, then.

    What do you prefer?

@ExTechOp ExTechOp changed the title [plugins/fingerpori] Update extract.js [plugins/fingerpori] Update extract.js (DO NOT MERGE) Nov 3, 2024
@ExTechOp
Copy link
Author

ExTechOp commented Nov 3, 2024

I decided to abandon this single strip correction, and created a new fork helsingin_sanomat with all five Helsingin Sanomat strips (the active ones fingerpori, fokit, viivijawanger, and the finished strips anonyymitelaimet and jaatavaspede). Please merge that fork instead of this.

@ExTechOp ExTechOp closed this Nov 3, 2024
@ExTechOp ExTechOp deleted the patch-1 branch November 3, 2024 12:14
@Olf0
Copy link

Olf0 commented Nov 3, 2024

Well, you could have simply done that (working on the other extract.js files) in your ExTechOp:patch-1 branch (which is entered by clicking on this, or by switching branches in the upper left when viewing your clone of this repository). (Now this branch does not exist any longer, because you deleted it.)

In general: In a branch of a cloned repo which was submitted as a PR to the original repo, all newer changes will also become part of the PR, until it is merged into the original repo.

P.S.: Note that GitHub uses the term "to fork" to what a git clone command does (and calls the result "a fork" meaning "a cloned repo"), the original and common meaning of "a fork" is "starting an independent project based on the original one" (which also technically is usually initialised by a git clone command when using Git as SCM).
Also note that cloning a repository usually clones that repository with all its branches. For PRs usually a new branch in your cloned repository is created, but that is still simply called "branch" (not "fork").

P.P.S.: When you close a PR it cannot be merged (technically) without deliberately reopening it.

@Olf0
Copy link

Olf0 commented Nov 3, 2024

This PR has been superseded by PR #105.

@Olf0 Olf0 changed the title [plugins/fingerpori] Update extract.js (DO NOT MERGE) [plugins/fingerpori] Update extract.js (superseded by #105) Nov 3, 2024
Olf0 pushed a commit that referenced this pull request Nov 3, 2024
… (https://www.hs.fi/sarjakuvat/), also checked and corrected their metadata.  Also rectify retrieval for "Anonyymit Eläimet" (anonyymitelaimet) and "Jäätävä Spede" (jaatavaspede) even though these strips have finished.

This PR addresses 2/3 of issue #103 and supersedes PR #104.
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