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

Item Group API #40

Merged
merged 24 commits into from
Jan 22, 2022
Merged

Item Group API #40

merged 24 commits into from
Jan 22, 2022

Conversation

OroArmor
Copy link
Member

@OroArmor OroArmor commented Oct 11, 2021

A port of Fabric's Item Group API with enhancements to increase usability and versitility.

@OroArmor OroArmor mentioned this pull request Oct 14, 2021
9 tasks
@i509VCB i509VCB added hacktoberfest-accepted library: item Related to the item library. new: library A pull request which adds a new library. new: module A pull request which adds a new module labels Oct 19, 2021
Co-authored-by: LambdAurora <aurora42lambda@gmail.com>
Co-authored-by: LambdAurora <aurora42lambda@gmail.com>
Copy link
Contributor

@LambdAurora LambdAurora left a comment

Choose a reason for hiding this comment

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

Last review got kinda lost with the force-push.

@LambdAurora LambdAurora added this to In Progress in Launch Roadmap Nov 13, 2021
@LambdAurora LambdAurora added this to the Initial release milestone Nov 13, 2021
@OroArmor OroArmor changed the base branch from 1.17.1 to 1.18 November 29, 2021 21:35
Copy link
Contributor

@LambdAurora LambdAurora left a comment

Choose a reason for hiding this comment

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

I will start to believe that I'm blind, but, qsl_ on namespaces isn't really used in any of the already merged modules, it's quilt_ instead.

library/item/item_group/src/main/resources/fabric.mod.json Outdated Show resolved Hide resolved
@TheGlitch76 TheGlitch76 mentioned this pull request Dec 4, 2021
4 tasks
Copy link
Contributor

@LambdAurora LambdAurora left a comment

Choose a reason for hiding this comment

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

Almost all good!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

The itemgroup's id is used in data-gen paths so it having a . feels wrong to me, but it is valid so looks good.

…p/api/QuiltItemGroup.java

Co-authored-by: Juuxel <6596629+Juuxel@users.noreply.github.com>
@lenrik1589
Copy link

What is the current behavior when the fixed group is selected and you change the page
it was one of FAPI flaws, it would switch from fixed groups to first group in the page
also the flawed behavior is going to the first group regardless of whether you've gone to the next or previous page, it should go to the last if you turn the page back

@OroArmor
Copy link
Member Author

@lenrik1589 I've never seen this, but being the same base as Fabric, it probably has the same bug. Do you have an example or way to reproduce the bug that I could try out?

@lenrik1589
Copy link

just make more than 2 pages and switch around while having search or survival inventory tabs open to see the first one
the second one i think is kinda self-explanatory, so, switching from the second page i would expect to be in the brewery tab (or potions?) but the menu will go to the building blocks tab, not sure if it is still an issue, but in 1.16.1 it sure was

@OroArmor
Copy link
Member Author

Oh i understand now, that each page always goes back to the "first" group when switching pages. Not sure if I know the best way to fix this, but I can try to make something for it.

@lenrik1589
Copy link

since i am back to my new laptop i will also look into how i fixed this in my mod

Copy link

@lenrik1589 lenrik1589 left a comment

Choose a reason for hiding this comment

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

also, i don't know if this is in the scope of the API but it would be nice to see improvements to the search tab, like block tag search and overall input handling

@OroArmor
Copy link
Member Author

OroArmor commented Jan 12, 2022

also, i don't know if this is in the scope of the API but it would be nice to see improvements to the search tab, like block tag search and overall input handling

While I like this idea, I feel like it has two main issues:

  1. QSL is and API, not a feature mod, changing things like this could be confusing for users
  2. Because item and block tags are different, and a good way to check your item tags is the search, using block tags as well could cause confusion.

@TheGlitch76
Copy link
Member

also, i don't know if this is in the scope of the API but it would be nice to see improvements to the search tab, like block tag search and overall input handling

Definitely not in scope of QSL at this time, and this feature would also require moving item groups to be a core API, or else it would be inconsistently present.

@lenrik1589
Copy link

yeah, i see, it just felt that being able to search by item tags but not by block tags created inconsistent behavior in vanilla (i am working on porting what i had for 1.16.3 to 1.18.1 and it just seems that there had been questionable changes to mapping as well as game code)

@OroArmor
Copy link
Member Author

@lenrik1589 I updated the code so that tabs are now saved per page.

@lenrik1589
Copy link

oh, i didn't even think of having each tab remember what was open, it makes more sense than just setting tab to [some arbitrary position], yeah
sorry couldn't look earlier, had exam two days ago and so completely missed activity here

@OroArmor
Copy link
Member Author

no worries!

@LambdAurora LambdAurora merged commit 41873f8 into QuiltMC:1.18 Jan 22, 2022
@OroArmor OroArmor deleted the item-groups branch January 26, 2022 18:10
@OroArmor OroArmor moved this from In Progress to Done in Launch Roadmap Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period hacktoberfest-accepted library: item Related to the item library. new: library A pull request which adds a new library. new: module A pull request which adds a new module
Projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants