search

FanTranslatorsInternational / Kuriimu2

Kuriimu is a general purpose game translation project manager and toolkit for authors of fan translations and game mods.
GNU General Public License v3.0
320 stars 58 forks source link

'Clean eyes' expedition (part 1) #94

Closed HoldYourWaffle closed 3 years ago

HoldYourWaffle commented 3 years ago

First of all: as said many times before, these are just suggestions. All commits are pretty much self-contained and can (hopefully) easily be reverted individually.

These changes are intended to tidy up or streamline the codebase or API's, as seen by a newcomer. I have not been through the full codebase yet, but I feel like this is a good point for a first batch of feedback. There's a chance some changes don't make sense in the context of the entire codebase, please just say so if that's the case.

To make reviewing easier:

Initial batch

  1. c7c1a8b This is an old commit from when I was working on searching. It's a pretty basic "in-place" rewrite of SearchPattern.cs to make the code more bearable readable (at least in my opinion).
  2. 6da0901 The title pretty much says it all. This is the first entry in the series "some parts seem to think every plugin is a FilePlugin". Since PluginId and Metadata are common to all types of plugins, I moved them to a new super-interface. I'll make sure this new interface is used everywhere appropriate in later commits.
  3. 777338d & 96a11b8 As discussed earlier
  4. 42fe142 TSIA.
  5. d3c55ad Small semantic change that allegedly does something on Mac systems.
  6. f14802c ADE

Deprecating marker interfaces

  1. 41ee531 The title pretty much says it all. Instead of a clunky marker-interface with casts, we can use a simple no-op default interface implementation for RegisterAssemblies in IPlugin.

  2. 6171991 Controversial sibling of the previous commit. Deprecates the following marker-interfaces in favor of a throwing default implementation and reflective Can* properties:

    • IPluginState: ISaveFiles
    • IFilePlugin: IIdentifyFiles
    • IArchiveState: IAddFiles, IRemoveFiles, IRenameFiles, IReplaceFiles

    I wasn't entirely sure how to implement the CanDelete check, should I check for RemoveAll as well? With the default-implementations it's possible for implementations to only override one remove method, which is not ideal, but so are clunky marker interfaces.

    This change replaces clunky casts & null-checks with simple and clear Can*-checks, as well as more streamlined plugin implementations. I'm aware that this is probably my most controversial change so far, so I'm interested to hear your thoughts. This implementation is 100% backwards-compatible, and I will "gladly" update all plugins if approved.

    Note that, at least as far as I can tell, the "this operation is not supported" cases were never handled. Was this intended?

  3. ac9856b Similar to the smelly-cast note below, these actions only make sense inside an ArchiveFormInfo (at least as far as I know)

  4. 2587316 TSIA, for some reason default-interface-implementations sort of "don't exist" on types that don't override them...

While implementing this I noticed/uncovered some 'smelly' casts (at least in my opinion) regarding IPluginState and IStateInfo:

This can "easily" be solved by generify-ing IStateInfo.PluginState and FormInfo.StateInfo, but this has a lot more impact than I'm comfortable with, especially due to the lack of a wildcard generic in C#. I'll take a second look at this tomorrow, there's a good chance I missed something.

Second batch

Coming soonTM