Skip to content

Conversation

@veger
Copy link
Collaborator

@veger veger commented Mar 25, 2024

Another PR by @DaleStan (ShadowTheAge#168) which I use quite often.
I had to fully rebase it due to all the changes we made in YAFC-CE. I tested it a bit, and it seems to be working.

From the original PR:

Where logical (The Add production recipe, Add consumption recipe, Select raw recipe, and Add new milestone windows) this allows you to control-click to select multiple items. The currently selected items are highlighted, and you can control-click again to deselect them. After control-clicking at least one item, use the OK button to add all selected items and close the window.

image

If you do not control-click, the single clicked-on item is selected and added, and the window is closed, as before.

Note that I split the MessageBox refactoring (getting rid of singleton implementation) into a separate commit. If this change is not desired, we can easily get rid of it. It is required!

The original PR contains 2 more commits, of which I didn't fully understand ShadowTheAge@651c079 but it seemed unrelated to this feature.

@DaleStan
Copy link
Collaborator

For what it's worth, ShadowTheAge@651c079 was merged in ShadowTheAge#155, so it shouldn't be re-done here. I'm not sure why it shows up in ShadowTheAge#168.

@veger
Copy link
Collaborator Author

veger commented Mar 26, 2024

@DaleStan nice to see you here :) Hopefully you are not bothered by me re-applying your PR for YAFC-CE? I really like it, and am missing it a lot!

I didn't check if that commit was applied already, I just noticed it didn't belong to the PR. Maybe due to the merges that we done in the PR, GitHub got confused and showed it in the PR?

Edit The PR was squash merged, so git doesn't know these separate commits (that I skipped) are the same. A rebase of the original PR would remove them from the PR.

@DaleStan
Copy link
Collaborator

No, no problem at all. I've been away from Factorio and didn't even realize this fork existed until you tagged me.

We do need to include the MessageBox refactoring in this PR; if it's excluded and someone adds multiple duplicate recipes, the "Recipe already exists" notification will pick one of those recipes for all its notifications, instead showing the correct recipe for each warning. There's also an argument for removing that entirely, since you can see which recipes are selected in the Add Production/Consumption Recipe selection window.

@veger veger force-pushed the select-multiple-objects branch from 9433994 to 194793e Compare March 27, 2024 19:46
@shpaass
Copy link
Owner

shpaass commented Apr 6, 2024

@veger, this PR is not a draft, so I wanted to check with you if it's ready for a review.

@veger
Copy link
Collaborator Author

veger commented Apr 7, 2024

Yes it is ready for review. @DaleStan created to PR in the original YAFC project, I just moved it and rebased it on the YAFC-CE codebase.

@veger veger force-pushed the select-multiple-objects branch 2 times, most recently from 661be3d to 9640566 Compare April 7, 2024 11:37
@shpaass
Copy link
Owner

shpaass commented Apr 7, 2024

@veger did you go with Code Cleanup on these files? There are no newlines at the end in some of them, and our editorconfig says that there should be.

@veger
Copy link
Collaborator Author

veger commented Apr 7, 2024

No I didn't, I only used git commands to fetch, rebase and push into this project...

Edit Now I remember: I had to manually apply all the changes due to the big differences after changing the code style.

I'll do a cleanup commit I'll find the missing newlines and fix the commits themselves.

@veger veger force-pushed the select-multiple-objects branch from 9640566 to e64c310 Compare April 7, 2024 12:25
@veger
Copy link
Collaborator Author

veger commented Apr 7, 2024

done

Copy link
Owner

@shpaass shpaass left a comment

Choose a reason for hiding this comment

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

Functionality seems fine. Code-wise, I won't claim that I understood everything, but I saw nothing too out-of-place.

@shpaass shpaass merged commit 05fab33 into shpaass:master Apr 7, 2024
@veger veger deleted the select-multiple-objects branch April 7, 2024 19:42
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.

3 participants