Skip to content

Deterministically select the issue that represents the stack#202

Merged
maforget merged 2 commits intomaforget:masterfrom
AZweimiller:stack-sort
Sep 25, 2025
Merged

Deterministically select the issue that represents the stack#202
maforget merged 2 commits intomaforget:masterfrom
AZweimiller:stack-sort

Conversation

@AZweimiller
Copy link
Copy Markdown
Contributor

Handles a few edge cases that result in the "top of the stack" issue changing when refreshing a smart list. Fixes #68.

@github-actions
Copy link
Copy Markdown

Build Successful! You can find a link to the downloadable artifact below.

Name Link
Commit 36a4c62
Logs https://github.com/maforget/ComicRackCE/actions/runs/17996074448
Download https://nightly.link/maforget/ComicRackCE/suites/46230681444/artifacts/4100320754

@maforget
Copy link
Copy Markdown
Owner

You shouldn't add reference to Id or even the Comic to the ItemView. ItemView on it's own is in a base class that has no knowledge of Comic or Id. It's the reason you had to use a dynamic. That is why most of the code to determine the Comparer is in ComicBrowserControl in the GetStackColumnSorter method. It gets called in ItemView when StackColumnSorter?.Invoke(group.Caption) is invoked.

There is also a Chain extension method that calls the tie-breaker comparer when there is a tie, no need to duplicate code.

But I've noticed something, it may fix the problem when there are still situation like sorting via Number, etc. But it doesn't always match the sorting inside the stack (even when no grouping). The regular sort inside a stack only shows the latest setting, but remembers 3. So it does also use a ChainedComparer and sorts using all these.

But my function only uses the 1st, so to match the behavior of the inside of a stack we would need to take all 3 (non-null). That would reduce even more the problem without even needing to add a tie-breaker and would match the inside stack behavior. I don't know if it's worth to fix some very rare disparities.

Check the nightly build to confirm that my changes still fixes the issue on your side.

@AZweimiller
Copy link
Copy Markdown
Contributor Author

I appreciate the tips. This was my first time digging into the CR source so it was a lot to grapple with. I'm also more of a sysadmin guy so my coding experience is limited to basic bash, Python, or PowerShell scripts. I know it shows, so thanks for your cleanup.

@AZweimiller
Copy link
Copy Markdown
Contributor Author

Test build looks good over here.

As far as the other edge cases that you noted:

My 2c is that the goal of this PR is to stabilize the thumbnail selection. It succeeds in reducing visual artifacts and flickering as stack thumbnails change when refreshing a smart list. It also fixes the glitchy stack behavior in #68. The thumbnail selection logic can be improved in a future PR if there is demand. I suspect most people will just manually select their thumbnail issue by using "set as top of stack" if they don't like the chosen issue.

@maforget maforget merged commit a13ee69 into maforget:master Sep 25, 2025
1 check passed
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.

Stacking Glitch

2 participants