Refactoring Code Under My Own Initiative?

I've been reading more about TypeScript and learning more about its similarities to C#, with which I am much, much more familiar. Among other things, TypeScript fully supports object-oriented programming. It is my understanding that object-oriented code can be easier to document, understand, and maintain, all things that are medium-term goals for Joplin. I don't know exactly what preflight testing Joplin uses, but object-oriented programming could also make the code easier to test, as well.

Would it be a reasonable undertaking for me to refactor small chunks of Joplin's codebase in furtherance of those goals? I'm asking because it's something I would like to take initiative on, and BUILD.md recommends pre-clearing independent initiatives in order to avoid wasting labor. I'm completely comfortable working independently out of everyone's way in my own fork. I'm all set up in my workspace, and I'm ready to go!

I am excited about contributing, so I look forward to your response. Thanks!

2 Likes

Yes TypeScript is great, we've started using increasingly (about 25% of the codebase is now TypeScript) and it makes development much, much easier.

I think certain features would not even have been possible without it. What I like is that you can make large scale refactorings, and the compiler will tell you any class or function call that needs to be updated. Whenever I convert a file from JS to TypeScript it also often reveals various subtle bugs and oddity. Overall it makes the codebase cleaner and easier to work with.

What kind of refactoring do you have in mind? You want to convert certain JS files to TypeScript? Or something more?

1 Like

Well, as you know I've been poking around in the theme engine. I tried generating a UML diagram in VS Code from Theme.ts, it, well, didn't do anything, so I was thinking of starting by making it so that it does.

My broader goal is to abstract the theming engine to make it easier and safer to develop new themes, and eventually to make it possible to update the theme colors on the fly in response to system events.

(Something I was thinking might be a safe eventual approach is ripping off the GtkStyleContext API so as not to reinvent the wheel, especially since GTK Styles are just CSS.)

How does that sound for scope? If the changes I make don't break existing functionality while accomplishing the above goals (and improving documentation) is that something you might be interested in integrating at some point down the line?

The themes have been cleaned up and converted to TypeScript not long ago and the types are now defined in this file: https://github.com/laurent22/joplin/blob/dev/ReactNativeClient/lib/themes/type.ts

Is there something more that's needed to create a new theme? Or is it because documentation could be improved?

If your goal is to create a dynamic theme like you described, I'm not sure refactoring theme.ts would make a difference. Perhaps explore your idea and try to get it working without any refactoring first? That might point you to whatever refactoring is needed. I expect you'll find the difficulty is in the logic of supporting dynamic themes, and making sure the components update when needed.

Yes, type.ts is one of the files I have been poking at (though as of yet I haven't modified it, just created new theme files that inherit from the Theme interface). The file that refused to generate a UML diagram was theme.ts.

My thought was to incorporate a larger portion of theme.ts into the Theme interface and eventually port the existing themes to CSS to make the interface more robust when a theme is modified. Presently the level of customization possible with userchrome.css is only a small fraction of the overall theme styling, while the hardcoded themes are far too exposed to the rest of the application.

The specific idea with ripping off the GtkStyleContext API is that doing so might ultimately also make it possible to use minimally modified GTK themes in Joplin or, even, waaaaay down the line, facilitate creating a native GTK port, since Electron support for Linux theming is far more limited than it is for macOS or Linux. Abstracting away from CSS could also make it less hack-y to call Electron's native-theming API.

Obviously a large portion—possibly even the majority—of the work would be porting existing calls to theme.ts to any new API, but the theme engine as it currently exists seems rather brittle. Increasing the level of abstraction and modularization could not only allow for wider customization but also improve stability.

Again, I'm not asking you to do any of this for me. I'm asking for your tacit approval to start working on it myself, since I don't want to put in a ton of work without the expectation that it would at least get a fair hearing should I eventually submit a pull request. Does that make sense? I really do appreciate the time you are taking to engage in this communication with me.

Also I know people nag you about aspects of the UI, and further abstraction and modularization could make it easier for other people (like me) to respond to them.

I feel your proposal is too ambitious at this point to be honest.

since I don't want to put in a ton of work without the expectation...

But why put a ton of work for a first pull request? Whether your proposal is approved or not doesn't matter actually, we'll only know if the PR is acceptable once you have something to show - maybe it will be fine, maybe not, maybe it will be too big and I won't be able to review or won't have time.

If you'd like to contribute at this point why not start on something smaller to get familiar with the codebase? For example bug fixing is always useful and can give you a deep understanding of the codebase. There's 11 high priority bug at the moment: https://github.com/laurent22/joplin/issues?utf8=✓&q=is%3Aopen+is%3Aissue+label%3Abug+label%3Ahigh and 8 more in the backlog: https://github.com/laurent22/joplin/issues?q=is%3Aopen+is%3Aissue+label%3Abug+label%3Abacklog

1 Like

I've made a significant amount of progress if anyone wants to take a look. No, it isn't quite working, yet, but it's almost there! The files I've been working on are here:


One particular difficulty is that the two outward-facing functions from theme.ts are extremely loosely typed. I am trying to limit my work for the time being to the contents of the above-linked folder (other than theme.ts), and the relative lack of typing makes the two functions harder to reimplement and subsequently debug.

Like, for example, most variables that get passed to themeStyle() are type string, but not quite all. and most of the values returned are individual CSS attributes, but there are a handful of bundled JavaScript Objects thrown in just to keep things fun.

The currently breaking behavior is calling StyleProvider.get('globalStyle'), which seems like it should work, but it doesn't. Again, all of the build scripts run without error (though with plenty of dependency warnings), and my changes to the code passed the commit preflight, so... fun!

Ideally once I get this working I would like to replace the themeStyle() calls with enum values so that all values passed to it will be valid, as the constructor for StyleProvider iterates through the name enums and initializes them.

Another thought I have is that replacing all of the cached styles with actual objects from npm/css should further reduce the possibility of errors. Doing so would also help with converting the various themes from TypeScript classes to CSS and/or Sass, which would further the ultimate goal of making the theme system fully extensible for end users.

Probably the biggest reason I've avoided replacing all of the calls to themeStyle() is that I want to avoid integration collisions from modifying literally dozens of TypeScript files all over the application. That, and having to dig through the call stack to figure out what is being passed in each of those dozens of instances. I mean, ultimately this will be necessary; I would just like to get the reimplementation of the existing functions working first.

There's a lot of changes in there, but I can see at least one issue:

As you probably know you can define interfaces in TypeScript. So if we were to refactor this, that's what we'd use. So instead of this:

public static BackgroundColorActive1:string = 'backgroundColorActive1';
public static BackgroundColorActive2:string = 'backgroundColorActive2';
public static BackgroundColorActive3:string = 'backgroundColorActive3';
public static BackgroundColorActive4:string = 'backgroundColorActive4';
public static BackgroundColorActive5:string = 'backgroundColorActive5';

you'd have this:

interface JoplinStyle {
	backgroundColorActive1: string,
	backgroundColorActive2: string,
	backgroundColorActive3: string,
	backgroundColorActive4: string,
	backgroundColorActive5: string,
	// etc.
}

I think your style is maybe more like Java or perhaps C#, but in TypeScript we'd use the above.

Also we are now using styled-component for style. The code with buildStyle, etc. is deprecated.

But I still think this is a huge task as a first pull request.

You're free to give it a try and experiment, but I prefer to tell you there's only a very low chance it will be merged. Reason being it's low priority - there's a ton of higher priority stuff before even starting to think about refactoring this part of the code.

I just prefer to be clear so that you don't go too far and get disappointed if it doesn't get merged.

Regarding the weird classes, they were originally string enums, but eslint wouldn't accept having them split out into their own files, so I converted them to classes as a hack in order to be able to commit the code I was working on. I know it's weird.

I also recognize that refactoring functional code is an extremely low priority, and I haven't even gotten the reimplementations working, yet. I wouldn't expect to do a pull request until my code is lean and functional and actually does something new. I will try and keep my code even with dev so I can submit a pull request when I'm ready.

Also I don't quite get why you seem to see a pull request as an out-of-the-blue, one-and-done, all-or-nothing type deal. Obviously I'm going to continue to document my progress and solicit feedback on here as I work, so I won't be ambushing you or anything.

1 Like

FYI I burned out after my last comment here because, yes, I bit off more than I could chew and tried to reinvent the wheel. (I was also experiencing unrelated medical issues.) If my bravado in this comment thread (and my strident description of Electron's lack of native UI elements) is why you seemingly resent me, then, yes, I was wrong.

As a third party observer, I don't think @laurent's comments reflected any resentment. Seems he thinks it's a lot of effort on a very low priority project that will eventually require a lot of his time for review prior to merger. So he was just being upfront so that your expectations weren't higher than what he could deliver in terms of time and focus given the low priority. At least that's how I read his comments.

I suspect if you were to take a crack at one of the high priority bugs he linked to earlier, you could get him to focus on that PR much more easily.

I'm referring to this:

and this:

I never actually finished anything with the code referenced in this thread because, again, I burned out, both due to being overly ambitious and due to unrelated medical issues. If I went back to working on what I had been working on back in October, I would probably more or less start from scratch.