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

Move sprite loading to a separate private method (refactoring of _load) #9226

Closed
wants to merge 1 commit into from

Conversation

webdeb
Copy link
Contributor

@webdeb webdeb commented Jan 23, 2020

Launch Checklist

Just a bit of refactoring, since the code is very specific and it makes sense to put the logic out of _load to a private method.

  • briefly describe the changes in this PR

@webdeb webdeb requested a review from vakila January 23, 2020 21:28
@kkaefer
Copy link
Member

kkaefer commented Jan 24, 2020

Thanks for your contribution. This issue is a little more complex than just exposing this function, e.g. it doesn't cover cases where the same URL is added multiple times, or when requesting multiple sprites simultaneously. Additionally, it's unclear what happens when different sprites provide an image with the same name. There are also various notifications that don't get fired for subsequent sprite loads (ImageManager#setLoaded), which could leave partially parsed tiles in limbo.

We're planning to expand the way icons are supplied to support a bigger variety of use cases rather than just one sprite. However, due to the complexity of multiple sprite sheets or other image sources, we'll go through an RFC process to ensure that we've covered as many use cases as well as edge cases as possible before embarking on an implementation.

Given this explanation, I'm going to close this PR for now since we can't merge it at the moment, even with modifications. However, we're very interested in learning more about your use cases for having multiple sprites because it will help us design an updated sprite/image/icon system that covers your use case.

@kkaefer kkaefer closed this Jan 24, 2020
@kkaefer kkaefer mentioned this pull request Jan 24, 2020
@webdeb
Copy link
Contributor Author

webdeb commented Jan 24, 2020

@kkaefer ok, that are good points that you first will go through the RFC process, I like that.
Are you ok, to change it to a private _loadSprite method right now, as already stated, it also makes the _load method a bit cleaner?

@kkaefer
Copy link
Member

kkaefer commented Jan 24, 2020

That's a change that we'd probably accept. I assume your intention is to call this private method. Please refer to my explanation above for why that's not a good idea and will probably break in various edge cases.

@webdeb
Copy link
Contributor Author

webdeb commented Jan 24, 2020

That's a change that we'd probably accept.

awesome 🎉

I assume your intention is to call this private method.

Yes, for sure. 😃

Please refer to my explanation above for why that's not a good idea and will probably break in various edge cases

  • I totally understand the risks

@webdeb webdeb changed the title Expose loadSprite as style.method Move sprite loading to a separate private method (refactoring of _load) Jan 24, 2020
@webdeb
Copy link
Contributor Author

webdeb commented Jan 24, 2020

@kkaefer can you reopen? I forced-pushed the changes into this branch, but I guess changes are not tracked by closed PRs

@kkaefer
Copy link
Member

kkaefer commented Jan 24, 2020

Unfortunately, I can't reopen this PR since Github won't let me with the message "The wb-loadSprite-as-style-method branch was force-pushed or recreated.".

@webdeb
Copy link
Contributor Author

webdeb commented Jan 24, 2020

@kkaefer np #9231

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.

2 participants