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

Feature Request: Changes to the Network Terminal Screen #228

Open
sesgoe opened this issue Jun 1, 2024 · 4 comments
Open

Feature Request: Changes to the Network Terminal Screen #228

sesgoe opened this issue Jun 1, 2024 · 4 comments
Assignees
Labels
Enhancement New feature or request

Comments

@sesgoe
Copy link

sesgoe commented Jun 1, 2024

Is your feature request related to a problem? Please describe.
It's annoying when I open the Network Terminal Screen, and there are useless traders for me to click on. Some traders are completely empty (maybe they placed down the network terminal and forgot about it), and some traders set up some initial stock, and then stopped playing the server completely. Life happens! I'm not really complaining about the empty traders existing -- I'm complaining that I have no way to hide them on the Network Terminal UI.

The other issue I keep bumping into is that when I search for something on the Network Terminal screen, if I click into a trader, and click back out, that search text gets wiped out, so my workaround has been to keep it copied on the system clipboard so I can continually paste it back in as I'm hunting for whatever I'm looking for. Having a small bit of persistence (doesn't need to be more than during the duration of Minecraft being open) would be super helpful here.

Describe the solution you'd like
I made a small effort in this PR here: #227 that details what I was looking for. The only thing I considered adding but ultimately didn't was a "clear" button for the search prompt.

Here's the tiny 10-second clip from the PR detailing the changes:

small_network_terminal_demo.mp4

Describe alternatives you've considered
There isn't really an alternative solution for my first problem, and my second problem can sort of be handled by copy-pasting, but it is not ideal, and annoying in general.

Additional Context
Apologies if I offended by way of making a PR -- I certainly didn't mean to imply you couldn't make the change yourself. I have this implemented for myself client-sided on the server I play on, so for me personally, it's not a huge deal whether you implement it or not, but I pinged my PR out to the server community and got some support, so it seemed like making this a public change would be in the interest of a bunch of players.

We use your mod's traders + currency pretty heavily over in the https://www.curseforge.com/minecraft/modpacks/craft-to-exile-2 modpack community, and it's the single best trading experience I've ever had as a Minecraft player. So if nothing else, please know you've built a really fantastic mod that handles all kinds of edge cases, and it's an amazing piece of work. Thanks for building it.

@sesgoe sesgoe added the Enhancement New feature or request label Jun 1, 2024
@RaidenKoizuma
Copy link

yeah on big servers this is kinda necessary cuz players straight up have to go through every single shop one by one to figure out which ones are staight up empty or have no stock. needs that qol. also pr looked cool

@Lightman314
Copy link
Owner

Lightman314 commented Jun 1, 2024

I do plan on adding more filtering options to the terminal at some point, so the "hide empty traders" potion of this will probably be included with them. Though one option I've considered that would probably make things easier in the meantime is perhaps change the text color for the traders name depending on it's trade state (red for no trades at all, orange for all trades out of stock, white for "normal", etc.).

The "remember the last search" feature should also be fairly easy to implement in the same way that I have it set up to remember your scroll state (which it already does, and was a very necessary thing back before it took up the entire screen), so I'll make sure this at the very least gets added in the next update.

On another topic, I'm not actually upset or offended about the pull request, my main issue with the PR was more that things were coded in a somewhat sloppy way (well I say sloppy, but that could simply just be a difference in our coding methods and/or your ignorance of how my system is designed as a whole, etc.) that broke other compatibility features and more or less hard-coded it to only work with Item Traders. I like to keep my mod open to add-ons, which means that most things should be left to the traders discretion on how to handle them (which is why theITraderSearchFilter class exists, so that each trader can manually filter any custom added trader types to search results as required).

That and while I don't mind people looking through my code searching for potential issues/optimizations/suggestions, or even posting edited code as a suggestion, while this is an open-source project it's not necessarily an open-contribution project. Unfortunately Github doesn't have the ability to simply disable Pull Requests on a repository, otherwise I would already done so, as I personally don't see a reason to have them on a single-person project.

@sesgoe
Copy link
Author

sesgoe commented Jun 2, 2024

done so, as I personally don't see a reason to have them on a single-person project.

No worries! Yea, this is a long standing issue with github in general. My apologies for any annoyance caused. It might be worth sticking a note in your README or something that you generally don't accept feature PRs. I saw that there was a merge from another PR (granted, it was translation related) so I figured another PR was fair game.

Like I mentioned, this is my first foray into modding at all, so I expected the code to be sloppy/bad. PRs in my mind are a vehicle to start a good faith discussion about something new, and it seems like it accomplished that goal here! In the best case I learn something about how I should have written it, and a few of your comments here taught me a few new things, so thank you!

Thanks for considering the change, and I look forward to the screen reworks!

@Lightman314
Copy link
Owner

Trader colors now change based on trade count/stock count in v2.2.2.0.
I'll work on adding the additional filter options in the next update.

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

No branches or pull requests

3 participants