Skip to content
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

feat: Backwards container searching and resume search after match [skip ci] #2483

Merged
merged 6 commits into from
May 12, 2024

Conversation

ShadowCat117
Copy link
Contributor

I'm not certain how legacy handled the direction, whether it was by actually pressing the next/previous buttons but I think this works nicely.

Pressing enter will default to forward search unless on last page. Hovering the next/previous page slot will determine direction otherwise.

ContainerSearching.mp4

Closes #1714

@kristofbolyai
Copy link
Collaborator

Perhaps it should just do a full search, no backwards searching needed. Example: You are on page 5, search for something. It goes to the last page, 10, for example, then it uses the bookmarks to go to page 1, searches until page 5.

@kristofbolyai
Copy link
Collaborator

But I am not saying you should not keep this, maybe just add last page -> first page support, and search until you re-hit the starter page. (And this conveniently will return you to the page you started the search on, if no items are found).

@ShadowCat117
Copy link
Contributor Author

Will only work for personal storage containers as they're the only things with the quick jumping. Could be interesting to have for them though

@kristofbolyai
Copy link
Collaborator

Will only work for personal storage containers as they're the only things with the quick jumping. Could be interesting to have for them though

Okay. Do you want to do that now, or different PR?

@ShadowCat117
Copy link
Contributor Author

Okay. Do you want to do that now, or different PR?

I'll do it now, don't think it should be too difficult

@ShadowCat117
Copy link
Contributor Author

Looping.mp4

There is a bit of overlap now with this, the quick jumping feature and the model, so I think I'll merge the quick jumping logic into the model in a later pr

@ShadowCat117
Copy link
Contributor Author

Don't merge yet, seems to be an issue with auto search not continuing for non personal storage containers

@ShadowCat117
Copy link
Contributor Author

Thinking about it, maybe it's not a good idea. I forgot to make it loop backwards, but that will act weirdly when your final page isn't one of the destinations.

If you start on page 1 and you search backwards with your final page as 19, it'll jump to 17 then go forward to 19, only to then go backwards towards 1

@kristofbolyai
Copy link
Collaborator

Thinking about it, maybe it's not a good idea. I forgot to make it loop backwards, but that will act weirdly when your final page isn't one of the destinations.

If you start on page 1 and you search backwards with your final page as 19, it'll jump to 17 then go forward to 19, only to then go backwards towards 1

Oh.. Yeah, this would only work then, if you had forwards search only, and no backwards search. Should we still do this then..? This method is slower, but searches 100% of the container. Your method can be faster (if guided by the user), but won't be "autonomous".

@ShadowCat117
Copy link
Contributor Author

Perhaps a good alternative is shift+enter to jump back to page 1 and start the search there

@kristofbolyai
Copy link
Collaborator

Perhaps a good alternative is shift+enter to jump back to page 1 and start the search there

Okay, sure.

Copy link
Collaborator

@kristofbolyai kristofbolyai left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@kristofbolyai kristofbolyai changed the title feat: Backwards container searching and resume search after match feat: Backwards container searching and resume search after match [skip ci] May 12, 2024
@kristofbolyai
Copy link
Collaborator

Maybe merge this with #2485?

@kristofbolyai kristofbolyai merged commit fbe04ae into Wynntils:main May 12, 2024
1 check passed
@ShadowCat117 ShadowCat117 deleted the container-searching branch May 12, 2024 17:16
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.

Bank Search Behaviour
2 participants