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

Icons (various improvements part 2) #11

Merged
merged 5 commits into from
Jun 25, 2022

Conversation

arthomnix
Copy link
Contributor

  • Show icons next to the name and description
  • Disable the button for the currently selected tab to give a visual indication that it is selected
  • Update Fabric Loader and API again

Also give an indication that the tab is selected by disabling the currently selected button
@arthomnix
Copy link
Contributor Author

build failed because gradle couldn't find minotaur?
is it just me or is half the internet broken right now

this.icon = new Identifier(VTDMod.MOD_ID, this.name.toLowerCase());

if (MinecraftClient.getInstance().getTextureManager().getOrDefault(icon, null) == null) {
Thread iconDownloadThread = new Thread(() -> {
Copy link
Owner

Choose a reason for hiding this comment

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

Opening a thread for each icon doesn't sound very efficient. It'd probably be better to have one or two threads downlading every icon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried downloading on one thread and it's very slow to load the icons compared to each icon having its own thread, it's probably better to leave it like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Singlethreaded:
singlethreaded

Multithreaded:
multithreaded

Copy link
Owner

Choose a reason for hiding this comment

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

It still doesn't convince me to open so many threads. Besides, not every icon is visible as soon as you open a tab, so downloading could be done once the user can see the entry. Instead of opening a lot of threads and never using them again, you could use a thread pool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to use a thread pool, let me know if you would prefer a different type of thread pool

Copy link
Owner

@IotaBread IotaBread left a comment

Choose a reason for hiding this comment

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

Looks great!

@IotaBread IotaBread merged commit 96269bc into IotaBread:1.19 Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants