Added a "re-download" button to the mod management screen

[bl33p-bl00p]

Downloads based on the on-file url, not the url fetched from the online lookup. Useful for updating mods which haven't been officially published yet, or fixing corrupted mods.

[SomeTroglodyte]

Welcome! But I'm a nitpicker... These are for now unverified, I may edit this post later

• Borderline usecase - will the casual player be confused or use the button in a wrong manner?
• There is already an Update mod button inside (yes one of two- not perfect I know) ModInfoAndActionPane, so it should be possible to merge code and avoid (or use for both) that callback
• Laaaaarge lambdas in field initializers are ugly - these lines focus on what fields exist, initializers should be trivial: move callback to private function (and recall that onClick(::foo) is more efficient than onClick { foo() } - unless you need to tweak call signatures) - or move entire factory to companion
• "Re-download Mod" likely needs a translation template
• Hmmm... We got progress reporting back, this one will not? The 'other' update won't either.
• Modder wiki: ??? - Can you self-host and set your own url inside modoptions - would that have some value or would Unciv overwrite anyway? (forgot - does rewriteStuff only touch that for gh sources?)

[bl33p-bl00p]

Thanks for reviewing my PR! Don't worry about nitpicking; I am all ears. The main problem I had with those two classes is that ModInfoAndActionPane.kt has the buttons and uses the on-disk urls (for lack of a better word), and ModManagementScreen.kt has the download functions (private) and uses the queried urls from github yk looking for those tagged published mods. Honestly this "solution" is the 1st thing I found that worked but I believe you that there has to be a better way! :) With that said, here are some thoughts on your point.

• bordeline usecase - i think it's mainly people testing unpublished mods that would find it useful instead of manually typing in the url every time, but anyway if y'all don't want it then truly I would not be offended (although I'd keep it in my personal distro:)
• already an update mod button, yes, but what about second mod update button! -_- anyways the existing mod update button only shows up for published mods, and if you have an unpublished fork of a published mod, then it shows up but only downloads the published version of the mod. And that button is created by the class that only has the queried urls, not the actual urls that the actual mods on your device are from. I initially tried to give this button the full functionality if that makes sense, but I couldn't figure out how to get the on-device mod url to it. I just wanted to be able to tap a button to re-download a mod.
• lambdas - i didn't understand a single thing you just said, i was googling for ideas on how to cross this bridge and that's the first thing that worked but i do agree it looks ugly lol. (i did try some other ways to pass the functions or parameters across)
• translation - you're right, i didn't think about this but would fix it. i should have included it with the PR, my bad
• idk what you mean about progress reporting back, it switches to downloading then flashes downloaded. Do you mean that the ModManagementScreen won't know that the mod was re-downloaded and therefore won't take some action that it should?
• modder wiki - are you talking about this section: "Loading mods from other sources than github" - didn't find that useful..
  So in conclusion, the complicated interactions between these classes stumped me, but if you can point me in a better direction then I'll work on it! I don't mean to be "project" to y'all but I feel like I have something to contribute here and there with some guidance. :)

[SomeTroglodyte]

As I said, to make those "brainstorm" entries more useful, I would have to check sources myself, means invest more time than I can at the moment. Sorry.

complicated interactions between these classes stumped me

Not your fault. That's grown code soup. Mostly my fault, but whenever I think 'just rewrite the damn screen' (making it mostly a sortable grid where the distinction online/local/both is shown in the cells, not by segregating them, and a pane to its right encompassing the current buttons, filter, bottom description and everything already in ModInfoAndActionPane)... then I remember 'oh then you'd have to rewrite the entire backend too to get a grip on exception handling, on background job cancellation, and enforce a provider interface model where GitHub can be just one specialization...' - ouch.

[bl33p-bl00p]

lol well imho unciv is pretty clean for a highly complex, decade running, open source hobby project! My tech philosophy in life is increasingly "If it works, don't touch it!". :)
What you think about these ideas?

1. Long press the existing "Open Github page" button to re-download, instead of the new button that for some people may be clutter or confusing. But, then it's kinda hidden and how would you know it's there? Put a note in the Modder wiki?
2. I should be able to move the bulk of the lambda to a private function
3. Translation - I'll double check before I submit the PR but if there ends up being any new text, I'll update the translation template accordingly
4. I think since I'm using the existing onDownload function which calls reloadCachesAfterModChange, updateInstalledModUIData(repoName), and refreshInstalledModTable(), I guess the ModManagementScreen class gets the updates it needs. I'll test this a bit more to try and break it

[SomeTroglodyte]

OK, first find:

Unciv/core/src/com/unciv/ui/screens/modmanager/ModInfoAndActionPane.kt

Lines 132 to 140 in 8da4987

	fun addUpdateModButton(modInfo: ModUIData, doDownload: () -> Unit) {
	if (!modInfo.hasUpdate) return
	val updateModTextbutton = "Update [${cleanModName(modInfo.name)}]".toTextButton()
	updateModTextbutton.onClick {
	updateModTextbutton.setText("Downloading...".tr())
	doDownload()
	}
	add(updateModTextbutton).row()
	}

Called here:

Unciv/core/src/com/unciv/ui/screens/modmanager/ModManagementScreen.kt

Lines 614 to 617 in a58a032

	modActionTable.addUpdateModButton(modInfo) {
	val repo = onlineModInfo[mod.name]!!.repo!!
	downloadMod(repo) { refreshInstalledModActions(mod) }
	}

So - the parent already 'injects' a kind of re-download mod button into the ModInfoAndActionPane, doing the actual DL itself. That should be adapted/leveraged/copied for your usecase somehow. I guess the uses are mutually exclusive? Think so, yes. Then one could offer the button with changing text as long as any useable uri is known. Leads to the next investigation...

This here:

Unciv/core/src/com/unciv/logic/github/Github.kt

Lines 212 to 214 in 8da4987

	val overwriteAlways = repo.direct_zip_url.isEmpty()
	if (overwriteAlways || modOptions.modUrl.isEmpty()) modOptions.modUrl = repo.html_url

... not sure atm (don't wanna look, avoid prejudices) where your code gets its uri from, but it just might be it would only work within one Unciv run, so - we should probably persist the source up there in all cases. Question is, do we put the direct_zip_url into modUrl or do we generously add a persisted field to ModOptions which will exist only on "direct" downloads ... I would 6:4 tend to the latter. I'm also rusty about what 'massaging' happens after you provide a download uri directly, there are some checks to recognize actual github sources and dodify fields accordingly - I just recently did something to ease testing older versions of mods via the per-commit-archive uri, and don't remember what that would leave in which field. I guess I'll have to run some downloads to look...

... to be continued ...

[SomeTroglodyte]

Long press

That's a "copy link to clipboard" and has a tutorial mention - or it should. No, considering I now think the button would appear only for DLs via direct links, that's perfectly fine, no risk of confusion, and clear enough not doc is needed...

... and by now I'm sure we should persist the "direct zip" uri in ModOptions, as in my tests the modUrl field will in most cases get the repo "home" url correctly, and that's good, and needed for the button just mentioned. Also the code in Repo.parseUrl could use better commenting and could treat another link type... I think I'll wrap that up and this should base on that. Or...

Look. If you're well set up (Studio, local branches...) this should be easy:
Take this (branch presented as diff)
https://github.com/yairm210/Unciv/compare/master...SomeTroglodyte:RedownloadMod?expand=1
..., copy any way you feel comfortable with (I would add me as remote then do a direct checkout), test, compare, amend, whatever you see fit, and if you're happy you're entitled to present the result as your own, no need for attribution. The important parts, after all, are concept and quality assurance.

Tested superficially with this link: Caballero-Arepa/Barbarian-xp-farm@d35d01b
... Yes I went and improved the ability to DL old versions of mods for debugging purposes again ...

Note this would offer the button ONLY for mods loaded with the "Download mod from URL".toTextButton(), and then only when it wasn't a plain repo homepage link which could successfully be queried via the GH api. Links to branches, tags, commits or zips hosted elsewhere should work - but only after DL'ing once with that code.

[bl33p-bl00p]

Oh boy let me dig into this and update my branch with your ideas, thanks for the help!

[SomeTroglodyte]

That fun() = fun in there is a lesson in lambdas - or their equivalence with a sort of functions... I don't really know all technical details, or what specific nomenclature is used by the experts, but to me there's a fundamental level between functions on a class method level and all deeper-nested ones, as in the former won't do closures, while the latter are 100% interchangeable with lambdas... In that case I find the slightly more verbose fun style clearer to read than the lambda style. It's still a closure how the button parameter is accessed in the body. Have fun.

[EmperorPinguin]

Maybe related to this, how about always fetching updates based on the on-file url? There have historically been issues with mods not being updated by the maintaner so the mod had to be cloned, updated, and published by a different Github user, but with same name for compatibility (savefile editing has become difficult due to new security measures). Installing currently requires deletion of the old mod and reinstall from url the new mod, and "updating" the mod in mod overview screen overwrites the new mod by the old one (presumably matching on name and then taking the oldest/highest starred mod). Instead, one needs to update by deleting the mod and then doing a new install from URL, which is inconvenient and has been the cause of problems and confusion for users. See discussion history in "A Game for ages" thread: https://discord.com/channels/586194543280390151/1231258844789735514

[SomeTroglodyte]

same name for compatibility

Generally - no, the 'model' has always been: the name alone is the key, and the mods a set - highlander 'there can be only one'. That has been baked in into too much code to change easily. The usecase is valid though -> our trivial answer would be 'a running game isn't that valuable that you couldn't abandon it or finish with the unmaintained mod'...

• "matching on name" yes, by the api query order - and we ask for them in starred order. Possibly later arrivals overwrite ealier ones? Point is, the update mod marker and button are tied to the matching between the left and middle lists.
• I could imagine another declarative mod compatibility unique that states the relationship you mentioned - for renamed successors. Problem is the old author won't cooperate, and such a unidirectional declaration could be malicious... But if we had that, continuing a game changing the mod would be doable. Prompt 'hey that save is missing X, but Y seems to be compatible' on load. Then again - multiplayer.
• With my code above, once you do a direct download of anything other than just https://github.com/owner/repo, and it' your scenario with a listed mod of same name existing - you should get both buttons, 'update' switching repos and 'reload' updating the one you chose... I'd appreciate you to test that.
• Of course, in that situation one would have all the information because as I said a ModUIData instance is known for both the left and middle columns - one could even prompt 'which author do you want' - or simply prefer the left one. BUT - easier said than done, the DL code and more importantly the post-DL 'save repo info into modoptions' code rely on a Repo object, and the left column doesn't have one. GithubAPI.Repo.parseUrl could fetch one for you, costing an extra api query. Plus, at the place you'd do that lookup, you're still on the GL thread, so calling a suspend fun would require an ugly runblocking or moving code around... Better save enough to be able to reconstruct ModOptions -> Repo, and I think the only missing bit would be the branch... Even then, would only work for downloads done with the updated code...

You know what, those same-name mods might even be provoking UI bugs. There's something I haven't been able to pin down with the middle column going mad right after a download... Stuff appearing above 5Hex even though it still has most stars and sort still is by star... But every time I try to repro I can't. I see it only when I haven't got the debug breakpoints I'd need to investigate. And haven't got enough energy to hunt.

[bl33p-bl00p]

I haven't dropped this. Just work and home got busy this past week.

[SomeTroglodyte]

By now I am 90% convinced we should start saving the direct url and maybe some of the other info - like: did we recognize a branch url / tag / commit - right in Github.rewriteModOptions. Haven't got around to the doing part however. Using that for re-download would make things much easier. Would only work after that saving has run once - per mod, but still.