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

Css parameter follow up #1110

Merged
merged 8 commits into from
Jan 14, 2024
Merged

Conversation

inforithmics
Copy link
Contributor

Reference Issue

Follow Up of following Pull request
#1107

What does this implement/fix? Explain your changes.

Made the pull request binary compatible.
Added Release notes

@inforithmics
Copy link
Contributor Author

inforithmics commented Dec 17, 2023

The question is if there should be made an Open overload with an options object like this.
public static T Open<T>(string path, SvgOptions options) where T : SvgDocument, new()
public static T Open<T>(Stream stream, SvgOptions options) where T : SvgDocument, new()
With this additional settings won't break the binary compatibility and wouldn't create more and more overloads.

@mrbean-bremen
Copy link
Member

@H1Gdev - please have a look!

@H1Gdev
Copy link
Contributor

H1Gdev commented Dec 18, 2023

As mentioned in #1107, I am negative to add more arguments to Open method.
This PR has also add a further variation Open method...(I think it will cause confusion to users.)

@inforithmics
If this feature is needed, how about providing methods that can set external CSS ?

@inforithmics
Copy link
Contributor Author

I thought about it and I think it is the better solution providing an external function that applies a css. So I can apply an css without again loading the document I will change this pull request accordingly

@paulushub
Copy link
Contributor

The question is if there should be made an Open overload with an options object like this. public static T Open<T>(string path, SvgOptions options) where T : SvgDocument, new() public static T Open<T>(Stream stream, SvgOptions options) where T : SvgDocument, new() With this additional settings won't break the binary compatibility and wouldn't create more and more overloads.

The SvgOptions (or SvgSettings) class will be a good idea. Most of the current Open/Create extra arguments could be added as fields.
Also, it can be added through an additional SvgDocument constructor. The default construction will create the default SvgOptions class with default settings.

@paulushub
Copy link
Contributor

paulushub commented Dec 29, 2023

The thought on options is to include properties

using System;
using System.Collections;
using System.Collections.Generic;
#if NETCORE
using System.Diagnostics.CodeAnalysis;
#endif

namespace Svg
{
    public class SvgOptions : IDictionary<string, string>, ICloneable
    {
        private readonly IDictionary<string, string> _properties;

        public SvgOptions()
        {
            _properties = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
        }

        public string this[string key] 
        {
            get
            {
                if (_properties.TryGetValue(key, out string value))
                    return value;
                return string.Empty;
            }
            set
            {
                if (key != null)
                {
                    _properties[key] = value;
                }
            }
        }

        public ICollection<string> Keys => _properties.Keys;

        public ICollection<string> Values => _properties.Values;

        public int Count => _properties.Count;

        public bool IsReadOnly => _properties.IsReadOnly;

        public void Add(string key, string value)
        {
            if (key != null)
            {
                _properties.Add(key, value);
            }
        }

        public void Add(KeyValuePair<string, string> item)
        {
            this.Add(item.Key, item.Value);
        }

        public void Clear()
        {
            _properties.Clear();
        }

        public bool Contains(KeyValuePair<string, string> item)
        {
            return _properties.Contains(item);
        }

        public bool ContainsKey(string key)
        {
            return _properties.ContainsKey(key);
        }

        public void CopyTo(KeyValuePair<string, string>[] array, int arrayIndex)
        {
            _properties.CopyTo(array, arrayIndex);
        }

        public IEnumerator<KeyValuePair<string, string>> GetEnumerator()
        {
            return _properties.GetEnumerator();
        }

        public bool Remove(string key)
        {
            return _properties.Remove(key);
        }

        public bool Remove(KeyValuePair<string, string> item)
        {
            return _properties.Remove(item);
        }

#if NETCORE
        public bool TryGetValue(string key, [MaybeNullWhen(false)] out string value)
        {
            return _properties.TryGetValue(key, out value);
        }
#else
        public bool TryGetValue(string key, out string value)
        {
            return _properties.TryGetValue(key, out value);
        }
#endif

        IEnumerator IEnumerable.GetEnumerator()
        {
            return ((IEnumerable)_properties).GetEnumerator();
        }

        object ICloneable.Clone()
        {
            return this.Clone();
        }

        public SvgOptions Clone()
        {
            SvgOptions options = new SvgOptions();
            foreach (KeyValuePair<string, string> item in _properties)
            {
                options.Add(item);
            }

            return options;
        }
    }
}

@paulushub paulushub self-requested a review December 29, 2023 17:36
@paulushub paulushub self-assigned this Dec 29, 2023
@paulushub
Copy link
Contributor

paulushub commented Jan 6, 2024

@inforithmics Thanks for the updates.

  • For new public methods/properties please use interfaces IDictionary<T, T> instead of the class Dictionary<T, T> where possible.
  • Keep the old namespace, instead of the new format for consistency.
  • If possible, avoid the file level nullable option for now, we will look into it at project level.

@paulushub paulushub merged commit d5e5059 into svg-net:master Jan 14, 2024
7 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 14, 2024
…Samples Source Svg.Custom Tests doc docfx.json index.md license.txt make adding string css parameter binary compatible CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt added release notes CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt introduce svgoptions for css paramater CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt added obsolete to unnecessary overloads CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt added more svgoptions overloads CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt fixing build CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt Improved release notes
github-actions bot pushed a commit to H1Gdev/SVG that referenced this pull request Jan 14, 2024
…DME.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt make adding string css parameter binary compatible CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt added release notes CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt introduce svgoptions for css paramater CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt added obsolete to unnecessary overloads CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt added more svgoptions overloads CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt fixing build CONTRIBUTING.md Generators README.md Samples Source Svg.Custom Tests doc docfx.json index.md license.txt Improved release notes
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.

4 participants