Skip to content

chore: add extra screenshot keys to niri#1697

Merged
johnae merged 1 commit intomainfrom
push-rorqrzusppxl
Mar 30, 2026
Merged

chore: add extra screenshot keys to niri#1697
johnae merged 1 commit intomainfrom
push-rorqrzusppxl

Conversation

@johnae
Copy link
Copy Markdown
Owner

@johnae johnae commented Mar 30, 2026

No description provided.

@johnae johnae merged commit 5ae5414 into main Mar 30, 2026
2 of 3 checks passed
@johnae johnae deleted the push-rorqrzusppxl branch March 30, 2026 21:23
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Mar 30, 2026

Claude finished @johnae's task —— View job


Code Review for PR #1697

Review Checklist

  • Gather context and read the modified file
  • Analyze code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage
  • Provide comprehensive feedback

Summary

This PR adds additional screenshot key bindings (F11, Ctrl+F11, Alt+F11) to the niri window manager configuration, providing alternative shortcuts alongside the existing Print Screen keys.

Code Quality and Best Practices ✅

The implementation follows the existing pattern in the configuration file:

  • Consistent syntax and structure with other key bindings
  • Proper use of Nix attribute sets
  • Clean and readable formatting

Potential Issues 🔍

  1. Key Binding Conflicts: The F11 key is commonly used for fullscreen toggle in many applications. Using it for screenshots could cause conflicts:

    • Web browsers use F11 for fullscreen mode
    • Many video players and games use F11 for fullscreen
    • This could lead to unexpected behavior when these applications have focus
  2. No Documentation: The PR lacks a description explaining why these additional keys were needed or what problem they solve.

Performance Considerations ✅

No performance concerns - key bindings have negligible performance impact.

Security Concerns ✅

No security issues identified. The screenshot functionality is standard and doesn't expose any sensitive operations.

Test Coverage ⚠️

  • No tests visible for the configuration changes
  • For Nix configurations, testing typically involves building and deploying the configuration
  • Consider adding a comment in the configuration explaining the rationale for duplicate bindings

Recommendations

  1. Consider using different keys: Instead of F11, consider less commonly used function keys like F12, or combinations like Super+F11 to avoid conflicts with application-level shortcuts.

  2. Add inline documentation: Consider adding a comment explaining why both Print and F11 keys are mapped to screenshot functions:

    # Alternative screenshot keys for keyboards without Print Screen
    "F11".action.screenshot = [];
  3. Verify no conflicts: Check if any other applications in your setup use F11 for critical functions.

Conclusion

The implementation is technically correct and follows the established patterns. However, the choice of F11 as an alternative screenshot key may cause usability issues due to conflicts with common application shortcuts. Consider using different keys or documenting the specific use case that requires these duplicate bindings.

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.

1 participant