search

sminez / penrose

A library for writing an X11 tiling window manager
https://sminez.github.io/penrose/
MIT License
1.26k stars 88 forks source link

Ability to cycle through tags #282

Closed griccardos closed 6 months ago

griccardos commented 1 year ago

implements issue #280 Adds two functions next_tag and previous_tag which can be called to cycle through tags let me know if you need me to make any changes

sminez commented 1 year ago

Can you add unit test for the new functionality please? There should be plenty of examples of existing tests to base them on in the that file

griccardos commented 1 year ago

Have added a test for cycling forward, backwards and wrapping around

sminez commented 1 year ago

@griccardos can you please rebase to develop and run rustfmt and clippy locally to address the issues that are being raised in the PR checks? Once that's done and the two changes I've requested are handled I think this looks like it should be good to merge :+1:

griccardos commented 1 year ago

I have done the changes as requested. Sorry, i forgot to do the fmt, because i was using helix editor. But it seems to be happy now. Let me know if i did the rebase correctly, its not something I do very often...

sminez commented 1 year ago

Looking at this again, I'm not sure about the use of unwrap_or_default if I'm honest. The current tag must be present in ordered_tags: if not then something has gone seriously wrong, and defaulting to the first tag in that instance feels like a) odd behaviour to provide in that case and b) masking a much bigger issue.

Can you replace both instances of unwrap_or_default with an expect please? The message should be something along the lines of "current tag is a known tag".

I'm not super happy about this sort of state manipulation being outside of the pure data structures really (they handle these sorts of invariants so API quirks like position returning an option that has to be Some aren't an issue). That said, I can't think of a way to do it differently and this is likely why I didn't provide these methods in the first place. The other state manipulation methods (mostly on a Stack) all have quickcheck tests to ensure that they behave correctly for arbitrary inputs and initial states. Doing that for these two new methods should be possible but is likely going to involve a bit of set-up to get it working smoothly I suspect.

griccardos commented 1 year ago

Sure, I can change to expect if that's what you think is best. I thought a fallback to a reasonable default seemed to be better option than a crash, even when it's never expected (excuse the pun), hence why I went for the unwrap_or_default. Will make the changes this weekend and test.

From: Innes Anderson-Morrison @.> Sent: Tuesday, October 17, 2023 12:24:28 PM To: sminez/penrose @.> Cc: Riccardo @.>; Mention @.> Subject: Re: [sminez/penrose] Ability to cycle through tags (PR #282)

Looking at this again, I'm not sure about the use of unwrap_or_default if I'm honest. The current tag must be present in ordered_tags: if not then something has gone seriously wrong, and defaulting to the first tag in that instance feels like a) odd behaviour to provide in that case and b) masks a much bigger issue.

Can you replace both instances of unwrap_or_default with an expect please? The message should be something along the lines of "current tag is a known tag".

I'm not super happy about this sort of state manipulation being outside of the pure data structures really (they handle these sorts of invariants so API quirks like position returning an option that has to be Some aren't an issue). That said, I can't think of a way to do it differently and this is likely why I didn't provide these methods in the first place. The other state manipulation methods (mostly on a Stack) all have quickcheck tests to ensure that they behave correctly for arbitrary inputs and initial states. Doing that for these two new methods should be possible but is likely going to involve a bit of set-up to get it working smoothly I suspect.

— Reply to this email directly, view it on GitHubhttps://github.com/sminez/penrose/pull/282#issuecomment-1766123814, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHINVLI3MWABBQPDPAN55XDX7ZMFZAVCNFSM6AAAAAA6ALT22KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRWGEZDGOBRGQ. You are receiving this because you were mentioned.Message ID: @.***>

sminez commented 6 months ago

Closing as there has been no further activity on this and the previous discussion highlights several issues with how this fits within the existing API.