Ability to cycle through tags #282
Conversation
|
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 |
|
Have added a test for cycling forward, backwards and wrapping around |
src/pure/stack_set.rs
Outdated
| self.focus_tag(new_tag); | ||
| } | ||
|
|
||
| ///Move focus to previous tag |
There was a problem hiding this comment.
Can you add a leading space to doc comments please and also add a quick explanation about the semantics of next/previous tag given that it might not be immediately obvious to users as this ordering is derived from workspace ID (so effectively creation order) rather than numeric or alphabetical order.
src/pure/stack_set.rs
Outdated
| @@ -1277,6 +1309,26 @@ pub mod tests { | |||
|
|
|||
| assert!(matches!(res, Err(Error::NoScreens))); | |||
| } | |||
|
|
|||
|
|
|||
| #[test_case(true,1,1; "forward")] | |||
There was a problem hiding this comment.
Clippy probably wont pick up on this but can you add spaces between the macro arguments here please?
|
@griccardos can you please rebase to develop and run |
|
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... |
|
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. |
|
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 GitHub<#282 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AHINVLI3MWABBQPDPAN55XDX7ZMFZAVCNFSM6AAAAAA6ALT22KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRWGEZDGOBRGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
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. |
implements issue #280
Adds two functions
next_tagandprevious_tagwhich can be called to cycle through tagslet me know if you need me to make any changes