sioyek: 2.0.0 -> 2.0.0-unstable-2024-01-25

[xyven1]

Gives access to many new configuration options, bug fixes, accessibility improvements, and features. Changelog: https://github.com/ahrm/sioyek/compare/main...development

Description of changes

• Updated package version, rev, and sha256.
• Added qtspeech dependency
• Removed patch as https://github.com/ahrm/sioyek/issues/804 was resolved
• libsForQt5.callPackage -> qt6.callPackage
• Added xyven1 to package maintainers.

Things done

• Built on platform(s)
  • [x] x86_64-linux
  • [ ] aarch64-linux
  • [ ] x86_64-darwin
  • [ ] aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • [ ] sandbox = relaxed
  • [ ] sandbox = true
• [x] Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• [x] Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• [x] Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
  • [ ] (Package updates) Added a release notes entry if the change is major or breaking
  • [ ] (Module updates) Added a release notes entry if the change is significant
  • [ ] (Module addition) Added a release notes entry if adding a new NixOS module
• [x] Fits CONTRIBUTING.md.

---

Add a :+1: reaction to pull requests you find important.

[pca006132]

We can add qtwayland as well, see https://github.com/ahrm/sioyek/issues/867

[xyven1]

@pca006132 Do you have a good idea of how to do that? I am happy to add it, but I think first priority is to get this update merged, as Sioyek is more than a year of features behind in nixpkgs.

[xyven1]

@podocarp Bumping this PR.

[pca006132]

I think you can just add qtwayland to buildInputs and it will work.

[pca006132]

But it seems that visual mark does not work well under Wayland for some reason. Perhaps we should not add qtwayland for now.

[xyven1]

Been daily driving this for a while, no problems. Still haven't tested on MacOS. I'm on wayland for what its worth.

[kilesduli]

Hi, I noticed that the current version of sioyek in nixpkgs has an issue due to the newer version of mupdf. When reading larger PDFs and using t to jump between sections, there's a memory leak and a white screen. The latest commits in both the development and main branches don’t have this problem, so I’m hoping the PR can be merged to fix this.

[stephen-huan]

Perhaps one of us can post on the NixOS discourse in one of the PRs ready for review threads?

I agree that it's been too long for a straightforward update.

[xyven1]

I will fix the merge conflicts today, so that this is PR is still viable. Again, I could update to an even more recent commit, but I (personally) noticed some regressions with the latest version, and feel that the version in this PR has most of the relevant changes (I'm open to any comments, and happy to bump the version if there is something important that got added or fixed).

In regards to adding to review threads, I have added this PR to the review threads in the discord multiple times, but never NixOS discourse, so that is another good option, if someone wants to do that.

[stephen-huan]

I will fix the merge conflicts today, so that this is PR is still viable. Again, I could update to an even more recent commit, but I (personally) noticed some regressions with the latest version, and feel that the version in this PR has most of the relevant changes (I'm open to any comments, and happy to bump the version if there is something important that got added or fixed).

Just out of curiosity, what have you noticed? I'm on ae733e9 which (was) latest on development branch, and before that, a3aeca4. To be clear, I'm not suggesting to bump the version, as I also haven't really noticed any improvements.

In regards to adding to review threads, I have added this PR to the review threads in the discord multiple times, but never NixOS discourse, so that is another good option, if someone wants to do that.

I can do this after the merge conflict is resolved.

[xyven1]

Just out of curiosity, what have you noticed?

I'm not sure which version I was on (it was the developement branch version sometime this past month), but there was a noticeable performance regression on scrolling (the document would not be loaded after scrolling, and would take time to load after being brought into view, which significantly disrupted the reading experience for me). My guess is that this was an intentional memory saving or efficiency change which was either being tested, or needed some polish. YMMV

I can do this after the merge conflict is resolved.

Appreciated.

[stephen-huan]

The latest commits in both the development and main branches don’t have this problem, so I’m hoping the PR can be merged to fix this.

@kilesduli I just noticed you mentioned the latest commits in development and main fix your issue; have you tested that this PR actually fixes your issue? It's pinned to 36cff8d on development which is relatively old.

[kilesduli]

The latest commits in both the development and main branches don’t have this problem, so I’m hoping the PR can be merged to fix this.

@kilesduli I just noticed you mentioned the latest commits in development and main fix your issue; have you tested that this PR actually fixes your issue? It's pinned to 36cff8d on development which is relatively old.

@stephen-huan I just tested 36cff8, and it resolved my issue. However, I found new problem. When scrolling through the table of contents by press , the scrolling is not smooth and frames will drop. Additionally, after clicking on a section to jump to it, the screen freezes, and I have to scroll with the mouse wheel to unfreeze.

For me I would prefer to update.

[stephen-huan]

Oh and @xyven1 apologies for the spam, but would it be possible to move this package to the by-name hierarchy to take advantage of the new mergebot? According to the documentation and readme it seems (1) it needs to be in pkgs/by-name (2) the person invoking the bot needs to be a maintainer and (3) the PR is generated by r-ryantm (nixpkgs-update). I'm not 100% sure whether r-ryantm will correctly identify the version for an unstable package though.

Not blocking for this PR and I can do it myself. And feel free to add me as a maintainer (lib.maintainers.stephen-huan).

[stephen-huan]

@stephen-huan I just tested 36cff8, and it resolved my issue. However, I found new problem. When scrolling through the table of contents by press , the scrolling is not smooth and frames will drop. Additionally, after clicking on a section to jump to it, the screen freezes, and I have to scroll with the mouse wheel to unfreeze.

For me I would prefer to update.

I'm not sure which version I was on (it was the developement branch version sometime this past month), but there was a noticeable performance regression on scrolling (the document would not be loaded after scrolling, and would take time to load after being brought into view, which significantly disrupted the reading experience for me).

Hmm, there seems to be a tension here. @xyven1 does your regression still happen on latest development (965499f)? If so you two can try to find something in the middle, I haven't noticed any problems personally.

[kilesduli]

@stephen-huan I just tested 36cff8, and it resolved my issue. However, I found new problem. When scrolling through the table of contents by press , the scrolling is not smooth and frames will drop. Additionally, after clicking on a section to jump to it, the screen freezes, and I have to scroll with the mouse wheel to unfreeze. For me I would prefer to update.

I'm not sure which version I was on (it was the developement branch version sometime this past month), but there was a noticeable performance regression on scrolling (the document would not be loaded after scrolling, and would take time to load after being brought into view, which significantly disrupted the reading experience for me).

Hmm, there seems to be a tension here. @xyven1 does your regression still happen on latest development (965499f)? If so you two can try to find something in the middle, I haven't noticed any problems personally.

I am also on commit ae733e9, which is the latest commit from three days ago. The problem I just discovered has not been encountered in this commit.

[xyven1]

Can i move it to pkgs/by-name since it is uses qt6.callPackage? Also I will check ae733e9

[xyven1]

Seems as though the latest commit in the dev branch works ok for me (still seems a little worse? but very hard to quantify). I will bump it to that version

[stephen-huan]

Can i move it to pkgs/by-name since it is uses qt6.callPackage?

I think so, you take qt6 as an argument and refer to the qt libraries by qt6.qtbase, for instance. See this package.

Seems as though the latest commit in the dev branch works ok for me (still seems a little worse? but very hard to quantify). I will bump it to that version

Thanks for your sacrifice : )

[xyven1]

Got it, I'll give that a shot

[xyven1]

Seems to work. Moved to by-name

[xyven1]

@kilesduli @stephen-huan Can you please run this PR, and see if everything is working as you would expect? If so, I will attempt to invoke merge-bot/

[kilesduli]

@kilesduli @stephen-huan Can you please run this PR, and see if everything is working as you would expect? If so, I will attempt to invoke merge-bot/请你运行这个 PR，看看一切是否按预期工作？如果是的话，我将尝试调用 merge-bot

Looks good to me. Works fine.

[stephen-huan]

@xyven1 it seems you might need to add passthru.updateScript and nix-update-script as an input? See, e.g.

https://github.com/NixOS/nixpkgs/blob/bc947f541ae55e999ffdb4013441347d83b00feb/pkgs/by-name/me/merge-ut-dictionaries/package.nix#L1-L5

https://github.com/NixOS/nixpkgs/blob/bc947f541ae55e999ffdb4013441347d83b00feb/pkgs/by-name/me/merge-ut-dictionaries/package.nix#L73-L78

and the contributing README. This seems to correctly generate PRs like https://github.com/NixOS/nixpkgs/pull/343670.

Reading the nix-update README I think the code for sioyek should be should be

passthru.updateScript = nix-update-script {
  extraArgs = [
    "--version"
    "branch=development"
  ];
};

If you could add this and also me as a maintainer, I'll post on the discourse, thanks!

[xyven1]

Will do.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/2016

[xyven1]

@stephen-huan I very much appreciate the research you did on this to get the PR all shaped up for another merge attempt. When I created the PR I really thought it was just a basic version bump, but I guess because it is to a dev branch it is a little more suspect... Anyways hopefully this finally gets merged.