Operation History & Package Icon Redesign

[mrixner]

Hey Marti!
Once again, GitHub won't let me make an empty draft PR and I can't work on this right now (and may not be able to for a while) but when I'm able, now that #3070 has been merged I would like to improve the operation history feature (as mentioned in the #3070 comment thread). Before that, however, I was thinking about reorganizing the whole package icon system (so I can easily use package icons in the operation history list rather than tenuously copy-pasting code from other parts of the application), since it seems like there's a lot of dead icon code throughout the codebase that I found when correcting #3100. I would like to reorganize / simplify the package icon system if that's alright with you? This would just be internal codebase changes, it shouldn't affect the user experience in any way. I'm not totally sure what I have in mind; I'll sit down and think about it when I can (and make a draft PR so you can veto anything you don't feel fits with the existing codebase). What do you think?

[marticliment]

I thought the package icon system was quite straight-forward... You only need to call one of the following methods, which return a file:/// or ms-appx:/// Uri pointing to the icon. This method will handle getting the icon's network url, downloading or updating the icon if necessary, caching it and downsizing it to prevent abusive disk space automatically.

class Package
{
	public virtual Uri GetIconUrl() // Returns the icon or a default one
	public virtual Uri? GetIconUrlIfAny() // Returns the icon or null
}

What did you find that was dead code? I can't think of it right now, but perhaps even I forgot this code existed 😅

[marticliment]

Yeah, that all seems to work great. I'm sorry, I just didn't quite understand the iconing system when I asked this. This function is the only thing I noticed that seems to be out of place, but it might be used somewhere; I'm not sure what the UniGetUI.PackageEngine.Package.Package class has on the PackageManagerClasses one or where it is used in place of the PackageManagerClasses one, which is probably where the function is being called.

I am sorry, something here is really wrong. One of this two projects was removed a long time ago from the solution, but did not get removed from the filesystem. I will remove it. The correct class is the one that inherits from IPackage

That's the other strategy I imagined, but it seemed like overkill to serialize the whole package object. (I think I just expected an invalid icon to show a default icon but that's stupid and not how it works...) However, that's a great point about the package details button and luckily, if a SerializablePackage bridge is added like for the installation options the complex settings can save it easily. Or the current Package object can be serialized - it feels like it should be able to given how the serializer works but I'm not sure what the reasoning for having the separate SerializableInstallationOptions_v1 is.

It might be overkill, but I reckon is the way to go if we want to have it work robustly and without having to add patches to the entire code for this to work (which is something I am trying to avoid more and more).
Regarding the SerializablePackage_v1, it is a mess and I wouldn't introduce it here, since it will export the entire package into JSON, including installation options and ingored updates. However, I suppose some alternative string ToSimplePackageJson() and static Package FromSimplePackageJson(string payload) could be implemented, to easily store packages as strings using the 5 essential variables (name, id, version, source and manager). The FromSimplePackageJson(string payload) static attribute would return a new instance of a Package for the given data.

[marticliment]

There shouldn't be more duplicate classes (as far as I have found)

[mrixner]

I am sorry, something here is really wrong. One of this two projects was removed a long time ago from the solution, but did not get removed from the filesystem. I will remove it. The correct class is the one that inherits from IPackage

I was wondering if it was something like this, since the IPackage one seemed more fleshed out.

It might be overkill, but I reckon is the way to go if we want to have it work robustly and without having to add patches to the entire code for this to work (which is something I am trying to avoid more and more).

Perfectly understandable, and a noble goal.

Regarding the SerializablePackage_v1, it is a mess and I wouldn't introduce it here, since it will export the entire package into JSON, including installation options and ingored updates. However, I suppose some alternative string ToSimplePackageJson() and static Package FromSimplePackageJson(string payload) could be implemented, to easily store packages as strings using the 5 essential variables (name, id, version, source and manager). The FromSimplePackageJson(string payload) static attribute would return a new instance of a Package for the given data.

Ahh, I didn't realize there already was a SerializablePackage_v1! That's a good idea, but could I make it using a class, SimpleSerializablePackage_v1 or something, just for simplicity (since the complex settings were designed to be able to serialize this directly and it seems more robust), or do you feel manually making the JSON is a better strategy?

[marticliment]

If you want to serialize to a class or manually, since this will be a small Json, I would personally do it manually (no class), but you can do it however you like it. One little thing, when serializing/deserializing JSON, make sure to pass to the serializer the argument CoreData.SerializationOptions or something similar, that adds support for trim and other stuff

[mrixner]

OK, thank you for your help!