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

Crafting menu search prefixes don't work #18573

Closed
zapa1928 opened this issue Sep 29, 2016 · 8 comments

Comments

Projects
None yet
4 participants
@zapa1928
Copy link

commented Sep 29, 2016

Win 7 bulid ver. 0.C-18865-g595705b (tiles) / bulid 5601

  • Unlock all recipes
  • bring up crafting menu
  • press / and type c:dehydrated or t:hammer or s:elec

It doesn't show any corresponding recipes.

It worked all right in bulid 5579 (i didn't download versions in between )

@illi-kun illi-kun added the <Bug> label Sep 29, 2016

@epsilon-phase

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

c:dehydrated doesn't look right to me, but I just tried to run it and it looks like you might be right.

c:food also seems to be broken... This is odd.

I'm rebuilding right now, I'm going to look at the source to see if a call was flummoxed, but the search code itself hasn't changed for some time.

Edit: I forgot that the code for searching a recipe is different than the general item search code. Also c:food isn't what I was thinking of either. :(

@epsilon-phase

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

It would appear that it was introduced here. dcb1d34

I don't see any indication that it wasn't intentional, but I don't understand why.

I suspect that @mugling intended to go about writing a new search function, but never got around to it.

I'm a bit peeved that this happened, as it seems that there was no new system in place to replace the one that was removed. This was one of my favorite parts of the gui actually.

@mugling

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

Yes, it's a known regression which I will be working on at some point. This fix might come after I update the crafting code to support mixed alternatives (eg. WRENCH 2 OR MECHANICS 2)

@mugling mugling self-assigned this Sep 30, 2016

@epsilon-phase

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

Okay, that's good then. How was it broken though?

@mugling

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

How was it broken though?

The crafting code is horrible - I wrote the shortest implementation that would suffice. A new implementation might avoid the prefixes in favor of a specific search dialog?

@epsilon-phase

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

I honestly prefer the prefixes, less mucking about.

I'd really prefer a unified gui for this, and the prefixes are, in my opinion, the best way to go about this. I think I'd be up for writing it if you would consider that.

What I meant by unified UI is that that using a search prefix is consistent with the other parts of the ui that support searching, such as the item list V.

@mugling

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

I'd really prefer a unified gui for this, and the prefixes are, in my opinion, the best way to go about this. I think I'd be up for writing it if you would consider that.

Yes such a PR would definitely be considerd - an idea with a (clean) implementation is always more persuasive than an opinion without one.

@epsilon-phase

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2016

@mugling, I've written a basic implementation, I'm not sure that it's quite up to snuff with the code quality (It's also not quite finished yet), but the parts I've written seem to work as I would expect.

There's a lot of change, and the code wasn't much fun to write, so I'd rather have some feedback on it before I'm completely finished.

epsilon-phase added a commit to epsilon-phase/Cataclysm-DDA that referenced this issue Oct 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.