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.

Sign up for GitHub

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

Jump to bottom

blackbox-terminal: support sixel #248682

Merged
merged 2 commits into from
Jan 15, 2024
Merged

blackbox-terminal: support sixel #248682

merged 2 commits into from
Jan 15, 2024

Conversation

linsui
Copy link
Contributor

@linsui linsui commented Aug 12, 2023

Description of changes

Use the same VTE as upstream for sixel support.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.05 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.

@ofborg ofborg bot requested a review from chuangzhu August 12, 2023 08:57
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Aug 12, 2023
chuangzhu
chuangzhu approved these changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@chuangzhu chuangzhu left a comment

Choose a reason for hiding this comment

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

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

1 package built:
  • blackbox-terminal

@chuangzhu chuangzhu added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Aug 21, 2023
@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/1080

@linsui linsui mentioned this pull request Sep 2, 2023
Open
kirillrdy
kirillrdy reviewed Nov 3, 2023
View reviewed changes
pkgs/applications/terminal-emulators/blackbox-terminal/default.nix
@@ -62,7 +63,17 @@ stdenv.mkDerivation rec {
];
buildInputs = [
gtk4
vte-gtk4
(vte-gtk4.overrideAttrs (old: {
Copy link
Member

Choose a reason for hiding this comment

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

should this be added do vte-gtk4 as an option ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if it makes sense but added anyway.

Copy link
Member

@kirillrdy kirillrdy Nov 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

sorry, that's not what I meant

I was suggesting to add sixel option to vte-gtk4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#252845

Copy link
Member

Choose a reason for hiding this comment

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

thank you for the link, after reading more about

I feel like blackbox should keep using stable vte.

usually it would be ok for have an optional feature, but the fact that it requires different version of vte sound like bad idea.

maybe someone is willing to maintain patches against stable version of vte ?

Copy link
Contributor Author

@linsui linsui Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we shouldn't wait until stable vte supports sixel. We just need to build blackbox against the vte version choosed by the blackbox dev.

Copy link
Member

Choose a reason for hiding this comment

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

haha I am so sorry for typo :-), please read previous post again

https://gitlab.gnome.org/raggesilver/blackbox/-/issues/273#note_1768201
as per that comment, only Flatpack is built with that version, and if people start reporting sixel issues to vte then sixel will be removed from blackbox

clear indication that it's too early to ship it in nixpkgs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only Flatpack is built with that version

IIUC, only Flatpak version is built with sixel support because the blackbox dev can only control that. And other distros can't use a different version of vte only for blackbox. In nixpkgs we don't need to ship it globally. We only ship it to blackbox users behind an experiment option. It's the choice of upstream.

Copy link
Member

@kirillrdy kirillrdy Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ok. with this in mind

how about ?

  • make blackbox-terminal always use version of vte that's listed in Flatpack
  • make sixel support optional flag, that's disabled by default
  • need approval from blackbox-terminal maintainer

EDIT: disclaimer this comes with higher chance of blackbox-terminal build fails when vte derivation changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I flipped the option.

@ofborg ofborg bot requested a review from chuangzhu November 3, 2023 10:11
kirillrdy
kirillrdy reviewed Nov 6, 2023
View reviewed changes
pkgs/applications/terminal-emulators/blackbox-terminal/default.nix Outdated
@@ -62,7 +64,17 @@ stdenv.mkDerivation rec {
];
buildInputs = [
gtk4
vte-gtk4
(vte-gtk4.overrideAttrs (old: lib.optionalAttrs sixelSupport {
Copy link
Member

Choose a reason for hiding this comment

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

option should not affect src, but only sixel support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean we should always use the same src?

Copy link
Member

Choose a reason for hiding this comment

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

make blackbox-terminal always use version of vte that's listed in Flatpack
make sixel support optional flag, that's disabled by default
need approval from blackbox-terminal maintainer

@ofborg ofborg bot added 10.rebuild-linux: 0 This PR does not cause any packages to rebuild and removed 10.rebuild-linux: 1 10.rebuild-linux: 1-10 labels Nov 6, 2023
kirillrdy
kirillrdy approved these changes Nov 6, 2023
View reviewed changes
Copy link
Member

@kirillrdy kirillrdy left a comment

Choose a reason for hiding this comment

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

@linsui appreciate you patience.

@chuangzhu can you have a look at recent changes ?

@chuangzhu
Copy link
Contributor

need approval from blackbox-terminal maintainer

I don't have a strong opinion against this.

this comes with higher chance of blackbox-terminal build fails when vte derivation changes

@linsui maybe add yourself as a maintainer and help tracking it?

@ofborg ofborg bot added 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Nov 6, 2023
@linsui
Copy link
Contributor Author

linsui commented Nov 6, 2023

No problem, added.

FliegendeWurst
FliegendeWurst reviewed Nov 6, 2023
View reviewed changes
Copy link
Member

@FliegendeWurst FliegendeWurst left a comment

Choose a reason for hiding this comment

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

Your last commit is not associated with your Github account.

@chuangzhu chuangzhu added 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Nov 6, 2023
@ofborg ofborg bot requested a review from chuangzhu November 6, 2023 10:04
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Nov 6, 2023
@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/1210

@wyndon
Copy link
Contributor

wyndon commented Jan 15, 2024

Can we get this merged ?

@NickCao NickCao merged commit 1f8d5f7 into NixOS:master Jan 15, 2024
24 of 25 checks passed
@linsui linsui deleted the sixel branch January 15, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FliegendeWurst FliegendeWurst FliegendeWurst left review comments

@NickCao NickCao NickCao approved these changes

@kirillrdy kirillrdy kirillrdy approved these changes

@chuangzhu chuangzhu Awaiting requested review from chuangzhu

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 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: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@linsui @nixos-discourse @chuangzhu @wyndon @kirillrdy @FliegendeWurst @NickCao