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

Ensure no item stack is being held before crafting in inventory menu #4779

Merged
merged 1 commit into from
Dec 6, 2017

Conversation

lacc97
Copy link
Contributor

@lacc97 lacc97 commented Nov 16, 2016

In the inventory menu, you can still click on the crafting preview to craft even if you are holding and dragging a different stack of items.

This patch fixes this by checking whether m_selected_item is either NULL or belongs to the 'craftresult' list (such that you can still easily craft multiple times by repeatedly clicking on the craft result box).

@est31
Copy link
Contributor

est31 commented Nov 16, 2016

Changes to the item system are always very prone to bugs, I've learned...

So better treat this PR with caution.

@lacc97
Copy link
Contributor Author

lacc97 commented Nov 16, 2016

Well, this doesn't really modify the item system, just the inventory menu GUI code.

// if there are no items selected or the selected item
// belongs to craftresult list, proceed with crafting
if(m_selected_item == NULL ||
!(m_selected_item->isValid() && m_selected_item->listname != "craftresult")) {
Copy link
Member

Choose a reason for hiding this comment

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

This equals to !m_selected_item->isValid() || m_selected_item->listname == "craftresult" - wouldn't that be much easier to understand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'll change it.

@est31
Copy link
Contributor

est31 commented Nov 16, 2016

Mhh yeah it looks harmless.

👍 looks good but someone independent from the PR author should test it.

@est31 est31 added this to the 0.4.15 milestone Nov 20, 2016
@juhdanad
Copy link
Contributor

juhdanad commented Dec 9, 2016

Tested, it works.

@paramat
Copy link
Contributor

paramat commented Dec 13, 2016

Seems a minor bug, nothing is lost if you place to inventory.
I noticed that if you are holding a stack then click the creative tabs for 'nodes' 'tools 'items' the stack held by the pointer changes, very minor and not much of a problem.

@paramat
Copy link
Contributor

paramat commented Dec 19, 2016

Removing from milestone as there's much more important stuff to work on.

@paramat paramat removed this from the 0.4.15 milestone Dec 19, 2016
@nerzhul nerzhul modified the milestone: 0.4.16 Dec 24, 2016
@paramat paramat removed this from the 0.4.16 milestone Dec 25, 2016

// if there are no items selected or the selected item
// belongs to craftresult list, proceed with crafting
if(m_selected_item == NULL ||
Copy link
Member

Choose a reason for hiding this comment

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

please add missing space

Copy link
Member

@nerzhul nerzhul left a comment

Choose a reason for hiding this comment

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

Okay for me

@nerzhul
Copy link
Member

nerzhul commented Jan 4, 2017

it seems travis build failed on GCC6 due to unit test, i relaunch it

@sfan5 sfan5 added Bugfix 🐛 PRs that fix a bug Rebase needed The PR needs to be rebased by its author labels Sep 11, 2017
@paramat
Copy link
Contributor

paramat commented Dec 6, 2017

@lacc97 approved so if you rebase we'll merge it immediately.

@lacc97
Copy link
Contributor Author

lacc97 commented Dec 6, 2017

Rebased.

@SmallJoker SmallJoker removed the Rebase needed The PR needs to be rebased by its author label Dec 6, 2017
@SmallJoker
Copy link
Member

Related PRs of the same code area: #6544 and #6520

@SmallJoker SmallJoker merged commit 2b5341c into minetest:master Dec 6, 2017
@lacc97 lacc97 deleted the craftcheck branch December 6, 2017 17:15
t0ny2 pushed a commit to t0ny2/minetest that referenced this pull request Jan 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants