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

Simplify IUser #2163

Merged
merged 2 commits into from
Oct 27, 2017
Merged

Simplify IUser #2163

merged 2 commits into from
Oct 27, 2017

Conversation

HebaruSan
Copy link
Member

Currently IUser has 6 public events, which makes it awkward to implement this interface. These events are only used inside NullUser, so at first blush, they belong inside NullUser as an implementation detail. However, all NullUser does with them is wire up the interface's functions to NullUser's own equivalent virtual functions. This is an unnecessary and cumbersome step of indirection with no advantages over simply calling those functions directly. (Technically this structure could allow third-party classes to inject listeners to various events in other places, but no code currently does this, so the need/advantage is debatable.)

This PR replaces this structure with a much simpler one and does some additional minor clean-up:

  • Remove the events from the interface, making it much simpler to implement in new classes, such as a new UI
  • Remove the events from NullUser
    • Make NullUser simply call the virtual functions from inside the interface functions
  • Remove the delegate definitions now that they're no longer used
  • Remove WindowWidth (not used even before this)
  • Rearrange the remaining members of IUser into a general-purpose group and a progress/updates group
  • (My editor auto-deletes trailing whitespace)

After these changes, Core\User.cs is much easier to understand, the IUser interface is much simpler to implement in new code, and existing code still works.

@HebaruSan
Copy link
Member Author

It looks like ReportDownloadsComplete / RaiseDownloadsCompleted / DownloadsComplete also do nothing. They're called from one place, but that call turns out to be a no-op, so that's removed as well in the latest commit.

@politas
Copy link
Member

politas commented Oct 27, 2017

My suspicion would be that as events ahave been removed from IUser's main sections, the editors have left them in the NullUser part by accident, not necessarily realising they were there. This PR seems to be removing a bunch of code, but I can't see any impact. Is it possible some of it is used in obscure commandline cases? I mean, the IDE/build process should flag that, I suppose.

@politas politas merged commit 387115c into KSP-CKAN:master Oct 27, 2017
politas added a commit that referenced this pull request Oct 27, 2017
@HebaruSan
Copy link
Member Author

"Obscure command line cases" is the story behind DisplaySelectionDialog / RaiseSelectionDialog / AskUserForSelection; the command line code uses this to talk to itself in some places, and it has no other current function, in GUI or Core. I wanted to keep this PR's scope narrow, though, so I left that alone.

I did text searches before removing the events, and then compiled the code after. I believe that would catch anything that's trying to use them. NullUser's virtual function interface is the de facto interface for IUser, and the rest of it is seems to be out of sight, out of mind.

@politas
Copy link
Member

politas commented Oct 27, 2017

NullUser is about allowing tests to run without interrupting the build process, I gather.

@politas
Copy link
Member

politas commented Oct 27, 2017

Great work, by the way, and very much appreciated! It's very easy for codebases like this to get filled up with unused cruft that no one dares touch.

@HebaruSan
Copy link
Member Author

HebaruSan commented Oct 27, 2017

Yes, NullUser does that, but it also has a strange dual role as a base class for UI-specific child classes. Currently GUIUser "is a" NullUser, as are Netkan.ConsoleUser and Cmdline.ConsoleUser, which means that all real errors/notifications the user sees in practice are routed through a class that is named after the idea of discarding them. That's another thing that I decided to leave alone for now.

@HebaruSan HebaruSan deleted the fix/simplify-iuser branch October 27, 2017 02:50
@politas
Copy link
Member

politas commented Oct 27, 2017

That seems odd. Well, if you want to tackle those issues as future projects, it would be most welcome. I find myself flailing in the wind trying to make sense of this code a lot of the time. See #1971 for a good example of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants