Skip to content

treewide: add testbed option for running command in UI #1110

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Flameopathic
Copy link
Contributor

@Flameopathic Flameopathic commented Apr 5, 2025

changes:

  • removes stylix.testbed.application.enable option
    • it is now assumed that, if the option is set, the application should be run
    • removes repeated lines
  • moves stylix.testbed.application to stylix.testbed.ui.application
    • if any options are set under stylix.testbed.ui, the UI configuration is set
  • refactors all existing application testbeds reflect the above two changes
  • adds stylix.testbed.ui.command.{text,useTerminal}
    • if the option is set, the command should be run
    • functions similarly to stylix.testbed.ui.application except can take any arbitrary command
    • has an option to run the command in a terminal
      • useful for terminal applications which require a terminal emulator for their full functionality

related: #319

Things done

Notify maintainers

@Flameopathic Flameopathic mentioned this pull request Apr 5, 2025
5 tasks
@Flameopathic Flameopathic force-pushed the treewide/add-testbed-option-for-running-command-in-UI branch 3 times, most recently from 6b6a71e to 7243d39 Compare April 5, 2025 22:30
@Flameopathic Flameopathic marked this pull request as ready for review April 5, 2025 23:55
@Flameopathic Flameopathic force-pushed the treewide/add-testbed-option-for-running-command-in-UI branch from 7243d39 to 623b022 Compare April 5, 2025 23:57
@Flameopathic
Copy link
Contributor Author

@trueNAHO do you have time to give this a quick review? I'd like to add some more application testbeds, but I am holding off until this has merged so that I don't have to do the same work multiple times.

@danth
Copy link
Owner

danth commented Apr 18, 2025

Just having one option which passes an attrset directly to makeAutostartItem would require less code (unless I've overlooked something), and potentially support more use cases

Copy link
Collaborator

@trueNAHO trueNAHO left a comment

Choose a reason for hiding this comment

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

I'd like to add some more application testbeds

Thanks helping out :)

but I am holding off until this has merged so that I don't have to do the same work multiple times.

Good idea.

Note that stacked PRs or simply well-structured patchsets allow progressing on dependant unmerged commits. AFAIK, this is common Git practice (at least in the Linux Kernel, which is arguably the intended way of using Git), although the GitHub interface neither supports nor really encourages this workflow. Keep in mind that this workflow requires frequent git rebases when commits in the patchset need to be changed, which is fairly difficult if you are not comfortable with git rebase merge conflicts.

Comment on lines 133 to 137
if (config.stylix.testbed.ui.application != null) then
{
inherit (config.stylix.testbed.ui.application) name package;
}
else if (config.stylix.testbed.ui.command != null) then
Copy link
Collaborator

Choose a reason for hiding this comment

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

This if-else construct implies that only one of them has an effect, with config.stylix.testbed.ui.application being prioritized. Either both being non-null should cause their consequences to be merged, or we explicitly disallow this with an

!(
  config.stylix.testbed.ui.application != null &&
  config.stylix.testbed.ui.command != null
)

assertion to prevent silently ignoring config.stylix.testbed.ui.command declarations.

Although merging the consequences is the most extensible, simply adding an assertion is also fine since we currently do not need this functionality. I leave the decision up to you.

Just having one option which passes an attrset directly to makeAutostartItem would require less code (unless I've overlooked something), and potentially support more use cases

Simply delegating pkgs.makeAutostartItem arguments is also an option, especially since the config.stylix.testbed.{command,ui.application} distinction is not directly apparent. Otherwise, the names could be somehow improved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just having one option which passes an attrset directly to makeAutostartItem would require less code (unless I've overlooked something), and potentially support more use cases

Simply delegating pkgs.makeAutostartItem arguments is also an option, especially since the config.stylix.testbed.{command,ui.application} distinction is not directly apparent. Otherwise, the names could be somehow improved.

Small correction: config.stylix.testbed.{command,ui.application} is actually config.stylix.testbed.ui.{command,application}, which may make their distinction slightly clearer (or maybe less clear), though I agree that it is hard to immediately understand the difference from their names. I am not sure what names would better differentiate them, though, and I believe that their option descriptions do a fair job of explaining what each does.

From what i can grasp about pkgs.makeAutoStartItem from its definition, it would require this set be duplicated with only one or to arguments being changed across every application testbed which runs an arbitrary command.

Also, doesn't the current upstream implementation already basically just pass the set directly to makeAutoStartItem?

@Flameopathic Flameopathic force-pushed the treewide/add-testbed-option-for-running-command-in-UI branch 3 times, most recently from 15fbd53 to f76677e Compare April 19, 2025 01:51
@Flameopathic Flameopathic force-pushed the treewide/add-testbed-option-for-running-command-in-UI branch from f76677e to 39649c2 Compare April 19, 2025 02:01
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.

3 participants