endless-sky
endless-sky copied to clipboard
endless-sky
Separate buttons for selling outfits and minables in TradingPanel
Feature: Fixes #8538 and fixes #7055, while improving the TradingPanel to avoid accidentally selling outfits and minables.
See also this comment in another thread: https://github.com/endless-sky/endless-sky/discussions/7525#discussioncomment-4190806
Feature Details
There are new [Sell Outfits] and [Sell Specials] buttons in the trading screen, with different keystrokes than [Sell All].
-
[Sell All]= shift-S to sell standard commodities -
[Sell Specials]= shift-P to sell minables and other special commodities -
[Sell Outfits]= shift-L to sell all outfits in the cargo hold.
I used "L" as a safeguard: people will rarely press L since it sends you to the landing screen. Also, it's on your other hand from S on a QWERTY keyboard. Selling outfits has a confirmation dialog that you can disable in the settings.
"To Do" List
People have made suggestions on how to improve this:
- [x] Add a
[Sell Specials]button that sells mineables and other special commodities. - [x] Expand the lower right part of the trading box to include the
[Buy All]and[Sell All]buttons. This will clarify that they refer only to normal commodities. - [x] Make hi-res versions of trading panel extension and embedded buttons
See the "Artwork Checklist" at the bottom of this description.
UI Screenshots
Usage Examples
- Don't sell your outfits by accident.
- Sell minables separately from commodities
Testing Done
Put outfits and commodities in my cargo bay and wandered from planet to planet seeing if things work as intended.
Automated Tests Added
Can't think of any.
Performance Impact
N/A
Artwork Checklist
- [x] I updated the copyright attributions, or decline to claim copyright of any assets produced or modified
- [x] I created a PR to the endless-sky-high-dpi repo with the
@2xversions of these art assets: https://github.com/endless-sky/endless-sky-high-dpi/pull/366 - [x] I created a PR to the endless-sky-assets repo with the original art assets: https://github.com/endless-sky/endless-sky-assets/pull/140
Pinging @williaji who requested both of the alternatives to this feature.
This seems to work really well. I've been carrying a collection of outfits in cargo and haven't managed to lose them yet. I've also been able to sell outfits on planets with and without outfitters. While it won't address the inability to recover after selling on a planet without an outfitter, it's a good fail-safe to keep it from happening. I'd love to see this merged.
The Sell All and Buy All buttons are slightly confusing now. You'd think All means all, but it just means Sell/Buy All Commodities. Endless Sky buttons can't get any wider than this. Does anyone have a suggestion on how to make the differences between the buttons more clear?
The
Sell AllandBuy Allbuttons are slightly confusing now. You'd thinkAllmeans all, but it just meansSell/Buy All Commodities.
Within the context of the full screen, the 'Sell Outfits' button makes a distinction between commodities and outfits even with the current buttons being somewhat vague. I'm not a new player, so I can't say that there wouldn't be some risk of confusion, but I think most folks would work it out fairly quickly. I think it also helps that 'Buy/Sell All' buttons are located under the relevant portions of the commodities display; it provides visual context. I doubt we are going to find a better wording given the space available.
I think most folks would work it out fairly quickly
If you have to work out a new interface, then it is poorly designed and needs to be changed. This one isn't just new to new players. Nobody has used it before. The entire community will have to "work it out," and I don't want them to have to do that.
What if after you clicked
Sell Allit turned intoSell Outfits?
I'm not sure if you're being facetious. That is the exact feature we want to remove.
Sorry, I just thought it would be a little clearer if the text changed. I guess that wouldn't make as much difference as I thought it could.
Sorry, I just thought it would be a little clearer if the text changed. I guess that wouldn't make as much difference as I thought it could.
It does, indeed. It makes people accidentally sell their outfits. You are correct though, that it makes the button meaning abundantly clear. I need a way to make the button's meaning clear without changing its meaning after you press it.
The one edge case that I've noticed in testing is that special commodities (i.e. mineables) are included in the 'Sell All.' They are called out in the text manifest in the left corner and will be sold with the rest of the commodities. I don't see this as a bug per se, but it is another case where we are overloading trade categories into a single function. Practically, this is unlikely to be a major problem. The worst case I can see is that someone has a mining job and accidentally sells the cargo. Given that there are few mining jobs and asteroids respawn, this seems primarily an inconvenience.
The one edge case that I've noticed in testing is that special commodities (i.e. mineables) are included in the 'Sell All.' They are called out in the text manifest in the left corner and will be sold with the rest of the commodities. I don't see this as a bug per se, but it is another case where we are overloading trade categories into a single function. Practically, this is unlikely to be a major problem. The worst case I can see is that someone has a mining job and accidentally sells the cargo. Given that there are few mining jobs and asteroids respawn, this seems primarily an inconvenience.
It would be easy to add a Sell Other button.
Or Sell Specials, since they're explicitly labeled as "special commodities."
Or
Sell Specials, since they're explicitly labeled as "special commodities."
A separate button would break out all the categories which makes trade commodities clearer.
Would it be possible to include the 'Buy/Sell All' buttons within the commodity frame? If the commodity table had an irregular border that included the 'Buy/Sell All' buttons it would make it clear that they are associated. The other two, being outside the border would be seen as separate.
Would it be possible to include the 'Buy/Sell All' buttons within the commodity frame? If the commodity table had an irregular border that included the 'Buy/Sell All' buttons it would make it clear that they are associated. The other two, being outside the border would be seen as separate.
I'm not sure what you're asking for. Could you please post an image explaining what you want?
The easiest way is to put rough boxes on an edited version of an image in my PR description. It doesn't need to be elegant. You can hide it behind a <details><summary> The image </summary> ... </details> block if you wish.
Oh! Okay, It is obvious to me now. I will ponder how to make an outline.
I put an outline around the buttons. How does it look?
Although I haven't yet managed to sell outfits in cargo by accident, I think it's a good safeguard against doing so. And just recently I had to temporarily move some mineables from cargo to storage and back again so I didn't automatically sell them.
An other alternative would be to include an [all]-button after every commodity: [buy] [sell] [all] Don't know how much that would interfere with the current setup but the changes so far look like a useful improvement and quite intuitive, especially if there is an additional explanation.
An other alternative would be to include an [all]-button after every commodity: [buy] [sell] [all] Don't know how much that would interfere with the current setup but the changes so far look like a useful improvement and quite intuitive, especially if there is an additional explanation.
Redoing the commodity trading area is outside the scope of this PR. If you want it to happen, please open an Issue with specific details. A mock-up user interface picture would help.
Redoing the commodity trading area is outside the scope of this PR. If you want it to happen, please open an Issue with specific details. A mock-up user interface picture would help.
This is what I had in mind:
I thought I first mention it here since an option to sell all of one type of commodity with a single click could also be a solution to the problem of selling things you do not want to sell (of course, one could also use <shift|ctrl|alt> but having a simple button for that is probably more convenient for most people). Was just a random idea, not very high on my wishlist. I think your current solution should work quite nicely in most instances.
This is working very well for me. It clearly segments the various trade types and requires you to make a choice to sell them. It's easy to both carry outfits and trade along the way. Each set of controls is clearly marked and the revised border collects the commodities block. "Specials" is vague, but that's what the game uses as we doubled up on the term "commodities." We might consider "Minerals" but I don't know if there is anything else that could fall into the "special" category. I'm delighted with this.
I would like a better option than [Sell Specials]. From the point of view of a new player, and the code, [Sell Minables] makes the most sense. However, the same category is what you would use for other non-standard commodities.
I thought I first mention it here since an option to sell all of one type of commodity with a single click could also be a solution to the problem of selling things you do not want to sell (of course, one could also use <shift|ctrl|alt> but having a simple button for that is probably more convenient for most people).
On the subject of modifying the trading table, I'd prefer to replace it entirely. There should be a better way to present that information.
@williaji - I just implemented the two features you asked for.
- Confirmation dialog if you press
[Sell Outfits]. You can turn this off in the settings. - A setting to disable that button on planets without an outfitter.
By default, you are allowed to sell outfits without an outfitter, and you get a confirmation dialog.
Having settings for these is a nice touch, as is the tutorial panel. This is a very thorough PR. Thank you.
I think the confirmation dialog box would be better without the pointer to the setting. I found it redundant to be reminded of a setting every time I sold outfits. You might mention the setting in the tutorial, but I would pull it from the dialog itself.
Since the purpose of this dialog is to prevent accidental sales, the default to should be 'Cancel' rather than 'OK'.
One other thought on the outfit sale confirmation. You might have it list the outfits to be sold the way the ship sale confirmation does. I will usually go to the player info screen and list the cargo before I sell outfits and that would save a step.
The outfit list is sorted in order from most valuable (value * count) to least.
I'm not really a fan of the preferences, because not everything should be a preference. So I'll propose the following: Show the confirmation dialog when clicking only on planets without an outfitter.
