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

sioyek: 2.0.0 -> 2.0.0-unstable-2024-09-29 #283813

Merged
merged 1 commit into from
Oct 7, 2024
Merged

Conversation

xyven1
Copy link
Contributor

@xyven1 xyven1 commented Jan 25, 2024

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

Description of changes

Things done

  • Built on platform(s)
    • 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
  • Tested, as applicable:
  • 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
  • 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot requested a review from podocarp January 25, 2024 19:18
@xyven1 xyven1 force-pushed the sioyek-unstable branch from 61193b1 to 2781e4c Compare March 4, 2024 00:41
@xyven1 xyven1 changed the title sioyek: 2.0.0 -> unstable-2024-01-25 sioyek: 2.0.0 -> 2.0.0-unstable-2024-01-25 Mar 4, 2024
Copy link
Member

@stephen-huan stephen-huan left a comment

Choose a reason for hiding this comment

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

Opened a few PDFs and seems to work.

Result of nixpkgs-review pr 283813 run on x86_64-linux 1

1 package built:
  • sioyek

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Apr 2, 2024
@pca006132
Copy link
Contributor

We can add qtwayland as well, see ahrm/sioyek#867

@xyven1
Copy link
Contributor Author

xyven1 commented Apr 7, 2024

@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
Copy link
Contributor Author

xyven1 commented Apr 7, 2024

@podocarp Bumping this PR.

@pca006132
Copy link
Contributor

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

@pca006132
Copy link
Contributor

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

@xyven1 xyven1 requested a review from stephen-huan May 13, 2024 16:25
@xyven1
Copy link
Contributor Author

xyven1 commented May 13, 2024

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

Copy link
Member

@stephen-huan stephen-huan left a comment

Choose a reason for hiding this comment

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

Still works for me.

Result of nixpkgs-review pr 283813 run on x86_64-linux 1

1 package built:
  • sioyek

pkgs/applications/misc/sioyek/default.nix Outdated Show resolved Hide resolved
@xyven1 xyven1 force-pushed the sioyek-unstable branch from 2781e4c to f62e23e Compare May 14, 2024 02:08
@xyven1 xyven1 requested a review from pluiedev May 14, 2024 02:12
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label May 22, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 4, 2024
@kilesduli
Copy link
Contributor

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
Copy link
Member

stephen-huan commented Sep 29, 2024

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
Copy link
Contributor Author

xyven1 commented Sep 30, 2024

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
Copy link
Member

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
Copy link
Contributor Author

xyven1 commented Sep 30, 2024

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
Copy link
Member

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
Copy link
Contributor

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.

@xyven1
Copy link
Contributor Author

xyven1 commented Sep 30, 2024

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

@xyven1
Copy link
Contributor Author

xyven1 commented Sep 30, 2024

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

@xyven1 xyven1 changed the title sioyek: 2.0.0 -> 2.0.0-unstable-2024-01-25 sioyek: 2.0.0 -> 2.0.0-unstable-2024-09-29 Sep 30, 2024
@stephen-huan
Copy link
Member

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
Copy link
Contributor Author

xyven1 commented Sep 30, 2024

Got it, I'll give that a shot

@xyven1
Copy link
Contributor Author

xyven1 commented Sep 30, 2024

Seems to work. Moved to by-name

@xyven1
Copy link
Contributor Author

xyven1 commented Sep 30, 2024

@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/

Copy link
Member

@stephen-huan stephen-huan left a comment

Choose a reason for hiding this comment

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

Built and opened a few PDFs, looks good to me.

Comment on lines 79 to 91
maintainers = with maintainers; [
podocarp
xyven1
];
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to add me as maintainer (lib.maintainers.stephen-huan).

@kilesduli
Copy link
Contributor

@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
Copy link
Member

stephen-huan commented Oct 5, 2024

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

{
lib,
fetchFromGitHub,
stdenvNoCC,
nix-update-script,

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

and the contributing README. This seems to correctly generate PRs like #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
Copy link
Contributor Author

xyven1 commented Oct 5, 2024

Will do.

@ofborg ofborg bot requested a review from stephen-huan October 6, 2024 02:11
@nixos-discourse
Copy link

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

Copy link
Member

@stephen-huan stephen-huan left a comment

Choose a reason for hiding this comment

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

Posted to PRs already reviewed, fingers crossed. Seemed to have a higher merged percentage than PRs ready for review.

@xyven1
Copy link
Contributor Author

xyven1 commented Oct 6, 2024

@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.

Copy link
Member

@phanirithvij phanirithvij left a comment

Choose a reason for hiding this comment

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

tested locally nix run github:nixos/nixpkgs/refs/pull/283813/merge#sioyek

@phanirithvij phanirithvij added the 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people label Oct 6, 2024
Gives access to many new configuration options, bug fixes, accessibility
improvements, and features.
@ofborg ofborg bot requested a review from stephen-huan October 6, 2024 16:33
@h7x4 h7x4 merged commit 199d79e into NixOS:master Oct 7, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants