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

Prioritize end date #488

Closed
wants to merge 7 commits into from
Closed

Conversation

DevilXD
Copy link
Owner

@DevilXD DevilXD commented Jun 2, 2024

@DevilXD
Copy link
Owner Author

DevilXD commented Jun 2, 2024

Everything here is all over the place, but nowhere where it should be. Besides the translation file changes that entirely don't belong here, I've noticed changes like can_earn_within(next_hour): being turned into can_earn_within_next_hour():, which is entirely unnecessary and limits possible future uses. filter_campaigns is copy-pasted from elsewhere in the code, which calls for a refactor, not code duplication.

For a start, I'd recommend trying to do some cleanup and removing the translation files from the diff, and reverting the can_earn_within_next_hour change.

@Windows200000
Copy link

I did revert the translation changes manually, I don't know why they are in the diff, The files should be the same.

The rest of the code wasn't written by me, I'll look into it.

@Windows200000
Copy link

update: You're right, can_earn_within_next_hour is completely unnecessary. I remember thinking about that, but probably forgot to ask the author. Reverted that.

The Language files had Windows instead of Unix line endings - reverted that too.

I searched around, taking individual lines from the function as well as the function name, and couldn't fine from where filter_campaigns would be copied.

@KevinNovak
Copy link

Any updates on this? Looking forward to using this setting

@DevilXD
Copy link
Owner Author

DevilXD commented Sep 4, 2024

I'd like to point out that this setting applies only to games that aren't added to the priority list, as ignoring said list would cause it to not make much of a sense. This also means that the "Priority only" setting has to be unchecked for it to work, otherwise only the priority list will be used.

Because of the above, having two separate checkboxes for this addition makes no real sense either, and it'd probably need to be transformed into a Combobox, from which the user can select either "Priority only", or one of several other modes for picking the non-priority campaigns order, first of which would be this "Prioritize soonest" option. Other modes could then be added later as needed.

Since my dark mode implementation isn't getting much of a progress due to difficulties I've encountered along the way, I'll try to work on this instead.

@DevilXD
Copy link
Owner Author

DevilXD commented Sep 6, 2024

A funny thing I've discovered/remembered/re-learned while working with the implementation of this feature, is that the miner's reload flow is:

campaigns -> ordered games list (based on priority list and active campaignsa) -> channels (live per game + collected ACLs from campaigns)

A single game can have multiple active campaigns to it, so the sorting process has to either be able to modify the existing priorities as they're generated, or the sorting has to be a two step process - first the campaigns are combined into per-game groups and sorted within each group, then the winner of this sorting is used in the 2nd sort, to generate priorities.

I'm still figuring out the implementation details, so I haven't decided which approach to take, and it'll take a while before this is available. Still, it seems like a simpler thing to implement than the dark mode PR I've been working on so far.

@DevilXD
Copy link
Owner Author

DevilXD commented Sep 9, 2024

I have a "working" prototype. Working as in something that's supposed to work, and doesn't crash - currently testing it. Before I can push it, I need to implement translations for the new options, for now it's just hard-coded English. It will be something that can be translated though.

picture

Should the inventory view remain being sorted by campaign's end time, or should it follow the overall application sorting?

@KevinNovak
Copy link

Thanks for working on this, looks great!

Should the inventory view remain being sorted by campaign's end time, or should it follow the overall application sorting?

I could see that being useful so you know what the miner will be working on next, but okay with leaving it as is for now and doing that as a separate enhancement? Either way is good for me

@DevilXD
Copy link
Owner Author

DevilXD commented Sep 10, 2024

"Separate enhancement" would require sorting them inside the GUI, which kinda sucks implementation-wise. The reason they're sorted in the GUI right now, is because they're sorted before being displayed at all - once they're added though, changing their place requires a whole lot of additional logic, keeping track of where each campaign row is, and updating that requested. As an easy modification, I can do one sort that cannot be changed, and for now, I'm leaving it at the start and end timestamps determining the campaigns order.

Just wanted to ask if someone saw it more fit to order them by the same order the miner uses to build it's internal wanted_games list, the order of which will determine the mining priorities from now on, after this feature is implemented fully.

@DevilXD
Copy link
Owner Author

DevilXD commented Sep 11, 2024

Closed by a30e8d3.

In case someone would be wondering what the new options are:

  • "Priority list only" - standard operation, only the priority list is considered. This is the old "Priority only" option checked.
  • "Ending soonest" - prioritize games from campaigns based on how soon the campaign is going to end.
  • "Low availability first" - prioritize games from campaigns based on the proportion of campaign duration to time remaining until the campaign ends. This the "better" ending soonest option, but I'm leaving that one as well.

Using any option other than "Priority list only" considers all campaigns/games that can be mined, working like the old "Priority only" option unchecked.

TODO:

  • Adjust translations.
  • Adjust documentation/README/in-app help tab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants