tryphotino / photino.NET

https://tryphotino.io
Apache License 2.0
833 stars 68 forks source link

Add overloads to OpenFile/SaveFile dialog filters for just regular string filters? #159

Closed grofit closed 1 week ago

grofit commented 6 months ago

The addition of native file open/save support is great but the API exposes custom tuple objects as the only way to provide filters for the dialogs and no string based alternative, which is what the tuple ends up getting converted to under the hood anyway.

So would it be possible to add an overload that allows you to pass in the string filter i.e

Image Files(*.BMP;*.JPG;*.GIF)|*.BMP;*.JPG;*.GIF|All files (*.*)|*.*

I am happy to raise a PR to do this myself but if I were to do it I would probably make a breaking API change and remove the current methods and have them expect the string versions of the filters then add some extensions/overloads which allow you to provide the more strongly typed versions.

The reasoning behind this is if you were to add an overload to allow a string you would currently need to map the string to the tuple, only for the tuple to be mapped back to the string again, so would make more sense to alter the underlying API to be more in line with the simpler string use case and add the more complex extension on top.

MikeYeager commented 6 months ago

@grofit Thanks for the suggestion. We're averse to breaking the existing interface. We'd be happy to add this capability, but it would have to be done without breaking what's in place. We don't feel it's worth it as the existing interface isn't difficult. e.g. var filters = new[] { ("Image Files (*.BMP;*.JPG;*.GIF)", new [] {"*.GMP", "*.JPG", "*.GIF"}), ("All Files (*.*)", new [] {"*.*"}) };

grofit commented 6 months ago

I appreciate its not that complex, but a lot of existing libraries that do the same sort of thing expect the string filter and in our case a lot of the existing infrastructure expect strings etc, and while I can convert that string to your format its only going to get converted back down underneath the hood.

Would you be open to allowing an overload so the high level API would remain the same for current callers but under the hood it would just do the transform as it currently does, then call the string variant? This way you can pass in either and the more complex (existing one) would be slightly more lightweight and easier to test as the responsibility for assignment/PInvoke would reside in the other method.

Im happy enough to do a PR for this work if you are happy with my proposed approach above.

MikeYeager commented 6 months ago

@grofit Yes, we would be happy to incorporate that as long as it doesn't break the current implementation. Thanks for your participation!

grofit commented 6 months ago

Sorry, after looking more into it I didn't realise I would need to change the native wrapping layer too, and that expects an array of strings which I assume get converted to a long string elsewhere, so the simplistic change I was expecting in this library would need propagating through other layers, so given that added complexity I will back off and just write a translator our side from a native string to your format.

Sorry to have wasted your time.