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

Repository management fixes #3876

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Conversation

HebaruSan
Copy link
Member

Background

CKAN can be configured to retrieve module metadata from multiple sources, called "repositories." The vast majority of users only use one repository. Alternate repositories are typically used to allow users to opt-in to bleeding edge releases of specific mods (e.g. MechJeb2 and Kopernicus).

Problems

  • Updating the registry retrieves repositories in order of their names alphanumerically, regardless of the priorities
  • The string Loading modules from {0} repository... was not internationalized

CmdLine

  • You can't edit the priorities from the command line
  • ckan repo list uses a weird format different from the tabular output of most other commands
  • Progress updates were hard-coded to show percentages only for strings containing the English word "download"
  • The priority of newly added repositories isn't set, so it's easy to get multiple that are 0
  • The string Loading modules from {0} repository... was displayed on many lines during updates

GUI

  • Newly added repos always have priority 0
  • Adding, deleting, and changing the priority of a repository is weirdly slow because the registry is being saved

Causes

This summary comment is false:

/// <summary>
/// Returns all the activated registries, sorted by priority and name
/// </summary>
[JsonIgnore] public SortedDictionary<string, Repository> Repositories
{
get { return this.repositories; }
// TODO writable only so it can be initialized, better ideas welcome
set { this.repositories = value; }
}

SortedDictionary sorts only by the key (in this case the name). (The specific sorting logic applied to the key can only be changed by passing an IComparer to the dictionary's constructor, which was never done here, but wouldn't have helped anyway if it had been since the priority is part of the value rather than the key). So Registry.Repositories has always only been sorted by name; the priority did nothing.

Changes

  • Now we sort the repositories by priority when we update the registry, so the priority will actually do something
  • Now repository priorities are cleaned up at registry load to guarantee they start at 0 and don't repeat
  • Now Loading modules from {0} repository... is internationalized

CmdLine

  • Now a new ckan repo priority command lets you set a repository's priority
  • Now ckan repo list uses a tabular output format like other commands
  • Now ckan repo add and ckan repo default set the priority of the new repository to the next available number
  • Now ckan repo add will refuse to add multiple repositories with the same URL
  • Now ckan repo forget adjusts the priorities of the remaining repositories after removing one to ensure they're unique and without duplicates or gaps
  • Now the "download" substring for progress update percentage display is internationalized
  • Now the string Loading modules from {0} repository... includes the word "download" so it will overwrite itself and show percentages

GUI

  • Now the columns in the settings window's repo grid are auto-sized to fit their contents
  • Now there's a confirmation popup before we delete a repo, so if you click Delete by accident you aren't faced with a need to find an obscure URL to fix it
  • Newly added repos have their priority set to the next available number
  • Trying to add a new repo with the same URL as an existing repo raises an error
  • Now while we save the registry after changing something repo-related, the hourglass cursor is shown and the UI remains responsive
  • The code for managing repo priorities is simplified for easier maintenance (confusing duplicated state storing the list in multiple places is eliminated)

@HebaruSan HebaruSan added Bug Something is not working as intended GUI Issues affecting the interactive GUI Core (ckan.dll) Issues affecting the core part of CKAN Registry Issues affecting the registry Network Issues affecting internet connections of CKAN labels Aug 7, 2023
Copy link
Member

@techman83 techman83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks @HebaruSan !

@HebaruSan HebaruSan merged commit 1d88407 into KSP-CKAN:master Aug 8, 2023
10 checks passed
@HebaruSan HebaruSan deleted the fix/repo-priority branch August 8, 2023 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is not working as intended Core (ckan.dll) Issues affecting the core part of CKAN GUI Issues affecting the interactive GUI Network Issues affecting internet connections of CKAN Registry Issues affecting the registry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants