LegoFigure11 / RaidCrawler

Raid Viewer for Pokémon Scarlet and Violet
GNU General Public License v3.0
168 stars 59 forks source link

Add match filter count, graceful stop and announcement text #114

Closed lfmundim closed 1 year ago

lfmundim commented 1 year ago

Issue 1:

I sometimes filter by shinyness and other stuff unrelated to it. The Crawler stops at a shiny found that I don't care about, but I would also like to know if other filters were a match, there is no visual indicator for it.

e.g: I am crawling for Shiny || 6iv ditto || 4iv 0atk 0spd ditto. It stops on a shiny I already have, so I don't want to go to it, I need to scroll through every raid to see if there are others (or Shift+Click scroll, which is fastest but doesn't feel like it is doing anything because it doesn't shift the focus raid, since there are no other matches).

Solution 1:

Add a text message next to Shiny: X saying Match: 0 to know at a glance how many of the 69 raids match any filter criteria

image

Issue 2:

I sometimes am crawling but I need to stop and nothing was found so far, so the bot is still running. If I click "Disconnect" the app gets angry at me throwing an exception.

image

Solution 2:

To mitigate that I added a "Stop Crawl" button (with a tooltip) that will finish the current iteration and then stop advancing dates

image

Issue 3:

Not all communities allow you to add webhooks to them to allow the usage of the bot, but they might be ok with the use itself. Whenever I announce the raid to a channel I need to manually type in details that are relevant for that raid.

Solution 3:

To mitigate that I added a "Copy Text" button that will put on my clipboard a sample message as similar as possible to the webhook one, without webhook-specific features (such as image or server emojis)

image image

Addendum

To acoomodate such changes, I had to do some UI changes, tried to keep it to a minimum.

If any of the changes need to be removed or just one of them can pass, please lemme know so I can adjust it.

LegoFigure11 commented 1 year ago

I'd rather we straight up replaced the shiny text with matches instead of having both

LegoFigure11 commented 1 year ago

As far as 2 is concerned, the code should be refactored to correctly use CancellationTokens instead of a workaround like you have implemented here

LegoFigure11 commented 1 year ago

I like the idea of 3, but the implementation could use some worth. It might be work splitting these off into 3 separate PRs so we can discuss each at more length individually?

e: on further thought you can just take a screenshot of the main window (or streamer view!!) and send that instead, so it also feels kinda unnecessary

lfmundim commented 1 year ago

I will be writing up separate PRs for 1 and 2

I like the idea of 3, but the implementation could use some worth. It might be work splitting these off into 3 separate PRs so we can discuss each at more length individually?

e: on further thought you can just take a screenshot of the main window (or streamer view!!) and send that instead, so it also feels kinda unnecessary

3 was kinda replaced by #119 , what do you think? It follows the logic of screenshotting, with a twist

lfmundim commented 1 year ago

As far as 2 is concerned, the code should be refactored to correctly use CancellationTokens instead of a workaround like you have implemented here

looking back at the code, I don't see how what's currently here is a workaround. The advance date loop is a do-while latch that checks for no cancellationToken, but variables to be set. If I cancel the AdvanceDate method it will just run again next iteration

image
LegoFigure11 commented 1 year ago

Yeah, it should also check for !CancellationToken.IsCancellationRequested, but it currently doesn't. That would be the best way to go about breaking out of the loop though.

lfmundim commented 1 year ago

Closing this PR in favor of 3 split PRs

Issue 1: #120 Issue 2: #121 Issue 3: #119