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

Stocks script based on pre-v50 stocks plugin #802

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

chawk0
Copy link

@chawk0 chawk0 commented Aug 26, 2023

This script is intended to clone the functionality of the pre-v50 stocks plugin from Falconne. The layout of the panels and hotkeys is nearly identical, with one exception being that filters are based on alt- hotkeys rather than ctrl- hotkeys.

Remaining features to implement:

  • filtering caged items and items-for-trade
  • toggling trade on items
  • zoom to a selected item
  • item grouping

@@ -0,0 +1,544 @@
-- a v50 script remake of the older stocks plugin
Copy link
Member

Choose a reason for hiding this comment

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

We should just name this "stocks", if it's a drop-in replacement and we aren't intending on resurrecting the plugin.

Copy link
Author

Choose a reason for hiding this comment

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

I had it named stocks.lua originally but somehow DFHack was bringing up an old stocks plugin description and upon running the command, I would get errors about possible compatibility problems, etc. For now, I just renamed it to stocks2 since I wasn't sure what the fix was.

Copy link
Member

Choose a reason for hiding this comment

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

Summarizing/expanding upon our conversation on discord:

The conflict is with the existing docs/plugins/stock.rst in the DFHack/dfhack repo. We may decide to keep that plugin as a host for item filtering logic that needs to be in C++ for performance reasons. If we need to move code to C++, another alternative might be to move the logic into the core library. We'll have to see what's best once we get a better handle on requirements.

For now, the easiest solution is to move stocks2.lua to gui/stocks.lua (and likewise for the docs file). That will resolve the naming conflict.

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.

This is a very good start, but the UX is not quite up to "modern" standards. Check out the AssignAnimal class in https://github.com/DFHack/dfhack/blob/develop/plugins/lua/zone.lua#L28 for example usage of HotkeyLabel, CycleHotkeyLabel, ToggleHotkeyLabel, FilteredList, searching, sorting, state management, and multi-select with shift-click.

})

function StocksScreen:init()
self:addviews({StocksWindow{}})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self:addviews({StocksWindow{}})
self:addviews{StocksWindow{}}

Copy link
Member

Choose a reason for hiding this comment

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

Lua provides some syntactic sugar for this case where the only argument being passed is a list/table

@@ -0,0 +1,544 @@
-- a v50 script remake of the older stocks plugin
Copy link
Member

Choose a reason for hiding this comment

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

Summarizing/expanding upon our conversation on discord:

The conflict is with the existing docs/plugins/stock.rst in the DFHack/dfhack repo. We may decide to keep that plugin as a host for item filtering logic that needs to be in C++ for performance reasons. If we need to move code to C++, another alternative might be to move the logic into the core library. We'll have to see what's best once we get a better handle on requirements.

For now, the easiest solution is to move stocks2.lua to gui/stocks.lua (and likewise for the docs file). That will resolve the naming conflict.

@@ -0,0 +1,14 @@
stocks2
========
Copy link
Member

Choose a reason for hiding this comment

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

the underline has to match the length of the string that it is underlining

========

.. dfhack-tool::
:summary: Alternative to the stocks screen based on the pre-v50 stocks plugin by Falconne.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:summary: Alternative to the stocks screen based on the pre-v50 stocks plugin by Falconne.
:summary: Stock management screen with search and filtering.

Copy link
Member

Choose a reason for hiding this comment

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

summaries should be ~53 chars


.. dfhack-tool::
:summary: Alternative to the stocks screen based on the pre-v50 stocks plugin by Falconne.
:tags: stocks
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:tags: stocks
:tags: fort productivity items

Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +351 to +360
function StocksWindow:show_qual_and_wear()
local quality_labels = {"Ordinary", "WellCrafted", "FinelyCrafted", "Superior", "Exceptional", "Masterful", "Artifact"}
self.subviews.filters_qual_wear:setText(
{
{ text = "-+", pen = COLOR_LIGHTGREEN}, { text = ": Min Qual: ", pen = COLOR_WHITE }, { text = quality_labels[min_qual + 1], pen = COLOR_BROWN }, NEWLINE,
{ text = "/*", pen = COLOR_LIGHTGREEN}, { text = ": Max Qual: ", pen = COLOR_WHITE }, { text = quality_labels[max_qual + 1], pen = COLOR_BROWN }, NEWLINE,
{ text = "Shift-W", pen = COLOR_LIGHTGREEN}, { text = ": Min Wear: ", pen = COLOR_WHITE }, { text = tostring(min_wear), pen = COLOR_BROWN }
}
)
end
Copy link
Member

Choose a reason for hiding this comment

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

This should be handed by the RangeSlider

Copy link
Member

Choose a reason for hiding this comment

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

You can copy the condition and wear sliders from here: https://github.com/DFHack/scripts/blob/master/internal/caravan/common.lua#L124

Comment on lines +362 to +387
function StocksWindow:show_filters()

local function get_pen(state)
if state then
return COLOR_WHITE
else
return COLOR_GREY
end
end

self.subviews.filters_label:setText(
{
{ text = "J", pen = COLOR_LIGHTBLUE}, { text = ": In Job ", pen = get_pen(filter_states.in_job) },
{ text = "X", pen = COLOR_CYAN }, { text = ": Rotten", pen = get_pen(filter_states.rotten) }, NEWLINE,
{ text = "O", pen = COLOR_GREEN}, { text = ": Owned ", pen = get_pen(filter_states.owned) },
{ text = "F", pen = COLOR_RED}, { text = ": Forbidden", pen = get_pen(filter_states.forbidden) }, NEWLINE,
{ text = "D", pen = COLOR_MAGENTA}, { text = ": Dump ", pen = get_pen(filter_states.dump) },
{ text = "E", pen = COLOR_LIGHTRED}, { text = ": On Fire", pen = get_pen(filter_states.on_fire) }, NEWLINE,
{ text = "M", pen = COLOR_BLUE}, { text = ": Melt ", pen = get_pen(filter_states.melt) },
{ text = "I", pen = COLOR_WHITE}, { text = ": In Inventory", pen = get_pen(filter_states.in_inv) }, NEWLINE,
{ text = "C", pen = COLOR_LIGHTRED}, { text = ": Caged ", pen = get_pen(filter_states.caged) },
{ text = "T", pen = COLOR_LIGHTGREEN}, { text = ": Trade", pen = get_pen(filter_states.trade) }, NEWLINE,
{ text = "N", pen = COLOR_GREY}, { text = ": No Flags", pen = get_pen(filter_states.no_flags) },
}
)
end
Copy link
Member

Choose a reason for hiding this comment

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

These should be handled by ToggleHotkeyLabel options

local utils = require("utils")

local item_desc_width = 38
local item_table = {}
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't have to track this state yourself. It will be accessible as the choices in the FilteredList

frame = {t = 3, l = 0},
view_id = "item_count_label"
}),
widgets.List({
Copy link
Member

Choose a reason for hiding this comment

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

This should be a FilteredList instead of just a List so you don't have to manage searching yourself.

Comment on lines +499 to +513
table.insert(list_contents, {
text = {
{text = full_item_desc, width = item_desc_width, rjustify = false, pad_char = ' '}, ' ',
{text = job_char, width = 1, pen = COLOR_LIGHTBLUE},
{text = rotten_char, width = 1, pen = COLOR_CYAN},
{text = owned_char, width = 1, pen = COLOR_GREEN},
{text = forbidden_char, width = 1, pen = COLOR_RED},
{text = dump_char, width = 1, pen = COLOR_LIGHTMAGENTA},
{text = on_fire_char, width = 1, pen = COLOR_LIGHTRED},
{text = melt_char, width = 1, pen = COLOR_BLUE},
{text = in_inv_char, width = 1, pen = COLOR_WHITE}, " ",
{text = quality_label, width = 13, rjustify = false, pad_char = ' ', pen = quality_pen},
},
ref = v.ref,
})
Copy link
Member

Choose a reason for hiding this comment

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

all this can be dynamically rendered by the text entry attached to the list choice. that would also make it responsive to changes to the item from outside of this tool's control.
Example: https://github.com/DFHack/dfhack/blob/develop/plugins/lua/zone.lua#L421

@myk002
Copy link
Member

myk002 commented Aug 27, 2023

Oh, and also please add a line to the "New tools" section of the changelog: https://github.com/DFHack/scripts/blob/master/changelog.txt#L29

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