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

Disable generation of id's unless necessary #559

Closed
burnpanck opened this issue Dec 18, 2016 · 4 comments
Closed

Disable generation of id's unless necessary #559

burnpanck opened this issue Dec 18, 2016 · 4 comments

Comments

@burnpanck
Copy link

I am a bit taken aback by the flood of id's that is generated by svg.js. In general, I see no reason for every element to have an id, and from quickly grepping through the source, I don't see the id's to be used much, except for retrieving elements, surprise, by id. Of course, there are cases where id's are required (e.g. gradients), and of course it's great that svg.js can manage the id's for us in that case. However, for most elements, the id's are just cluttering the generated markup. If there was an option to disable automatic id generation upon creation, and instead add a special forcing getter for the use inside svg.js where needed, would that cause any problems? Would a PR along that line be appreciated?

@Fuzzyma
Copy link
Member

Fuzzyma commented Jan 10, 2017

I wanted to comment that some time ago. Looks like I missed that. Sorry for that!

Ids are the most common way to identify elements in svg. The id everytime an element is created. To "disable" the id generation you would disable it for everything. Enabling it for only certain features (which are quite much e.g. masking, clipping, linking, images, symbols, use...) would require lots of unneccessary doublechecks. The load which is generated through the ids is in no way higher than the effort to manage those ids when they are not there.

Ofc all this would be encapsulated into the id() function but I am still not sure if this is teh right way.

@wout @dotnetCarpenter thoughts?

@burnpanck
Copy link
Author

Thanks for the response, and no worries, this is certainly not an urgent issue. I'd say from a performance point of view it wouldn't make much of a difference. The id's certainly do not cost much in terms of memory, and even less in computation. It's more of a cosmetic issue: it makes the generated markup hard to read. On the other hand, a single additional if statement upon each reading of the id should be tolerable too. Unfortunately I most likely won't have time to do a PR in the next few weeks, otherwise it would probably be easy to test.

@wout
Copy link
Member

wout commented Feb 25, 2017

It's a good idea to revisit this for 3.0.0. Maybe id's should be generated only if a reference is made. Like for gradients fills, clip-path, etc. Also, maybe clear id's if the reference is no longer required?

Fuzzyma added a commit that referenced this issue Apr 23, 2017
Instead they are generated when requested (#559)
@Fuzzyma
Copy link
Member

Fuzzyma commented Apr 23, 2017

@burnpanck I implemented this feature with 54362a8 in the 3.0 branch. It has to be tested if it has performance drawbacks but when I think about it - it reduces the time needed when creating objects so this is a big plus already. I will close this for now. Feel free to add your thoughts anyway.

@Fuzzyma Fuzzyma closed this as completed Apr 23, 2017
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

No branches or pull requests

3 participants