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

Add distance and value info to buildingplan item selection list #4312 #4956

Merged
merged 3 commits into from
Oct 4, 2024

Conversation

frogi16
Copy link
Contributor

@frogi16 frogi16 commented Sep 22, 2024

  • add option to sort by value
  • reorder sorting options to match ordering of colums
  • adjust width of existing columns

@frogi16
Copy link
Contributor Author

frogi16 commented Sep 22, 2024

Would solve issue #4312 originally requested in #4308

image

@frogi16
Copy link
Contributor Author

frogi16 commented Sep 22, 2024

FYI: @myk002, @phooenixdassmatte

EDIT: I just thought that some items might have the same name but differ in value because of values of material used to enhance the items (just compare sapphires to agats...). Tomorrow I will test what happens in such situation when my solution is applied, because it's possible the items are lumped together despite having different values.

@frogi16 frogi16 marked this pull request as draft September 22, 2024 22:10
@phooenixdassmatte
Copy link

phooenixdassmatte commented Sep 24, 2024

FYI: @myk002, @phooenixdassmatte

EDIT: I just thought that some items might have the same name but differ in value because of values of material used to enhance the items (just compare sapphires to agats...). Tomorrow I will test what happens in such situation when my solution is applied, because it's possible the items are lumped together despite having different values.

Hey there! Looks good, and still gives more information than vanilla.

I have no idea how df code works but an idea to include the encrusted object in the name?
So:
+<<brass chain>>+ enhanced with +polished topaz+

That way it would be a separate line of:

+<<brass chain>>+ enhanced with +polished diamond+

And would also give more information. We have big screens now so the added text would fit, but could maybe become problematic if it is encrusted several times haha. :D

@frogi16
Copy link
Contributor Author

frogi16 commented Sep 24, 2024

I managed to ungroup items with different values quite easily
image

@myk002 I would be grateful for code review.

@frogi16 frogi16 marked this pull request as ready for review September 24, 2024 20:37
@myk002
Copy link
Member

myk002 commented Sep 27, 2024

Ok, now that the craziness of the 50.14 release is behind us, I can catch up on code reviews. Thank you for your patience.

Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

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

Sorry the review took so long! Thank you for this addition!

plugins/lua/buildingplan/itemselection.lua Show resolved Hide resolved
plugins/lua/buildingplan/itemselection.lua Show resolved Hide resolved
plugins/lua/buildingplan/itemselection.lua Outdated Show resolved Hide resolved
plugins/lua/buildingplan/itemselection.lua Outdated Show resolved Hide resolved
plugins/lua/buildingplan/itemselection.lua Outdated Show resolved Hide resolved
plugins/lua/buildingplan/itemselection.lua Show resolved Hide resolved
…k#4312

 - adjust width of existing columns
 - reorder sorting options to match ordering of colums
 - save item name (desc) in dictionary items instead of using keys for
   this purpose
 - encode item value in dictionary key, this way ungrouping items with
   different values
@frogi16
Copy link
Contributor Author

frogi16 commented Oct 3, 2024

@myk002 I made post-review changes, please review and merge if everything is fine. I tested it quickly and seems OK.

Force pull mentioned by Github is just me rebasing on top of new changes from official repo, I will try to do it cleaner next time with separate branch.

@myk002
Copy link
Member

myk002 commented Oct 4, 2024

yeah, try not to rebase while the review is active since it causes all the comments to get lost and I lose the ability to see what's changed since the last review. Merging in from upstream is fine, though!

@myk002 myk002 merged commit 832069c into DFHack:develop Oct 4, 2024
14 checks passed
@myk002
Copy link
Member

myk002 commented Oct 4, 2024

when you have a minute, could you add your name to https://github.com/DFHack/dfhack/blob/develop/docs/about/Authors.rst ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants