Refactor deprecated part of ProtectedTermsFormatter #3720
Conversation
lenhard
added
status: ready-for-review
| @@ -42,7 +43,7 @@ | |||
| public static final List<Formatter> CASE_CHANGERS = Arrays.asList( | |||
| new CapitalizeFormatter(), | |||
| new LowerCaseFormatter(), | |||
| new ProtectTermsFormatter(), | |||
| new ProtectTermsFormatter(Globals.protectedTermsLoader), | |||
There was a problem hiding this comment.
Can you try to remove the dependency on Globals from the logic classes? I think it is better to have static constructor methods here (accepting e.g. the terms loader as parameter) instead of static lists. Not sure how many classes are using these fields right now...
There was a problem hiding this comment.
Turns out this is way harder than expected. There are several other static classes (e.g. Cleanups) that depend on this. I'll try to get this extracted somehow, but the current version definitely is not functional.
lenhard
removed
the
status: ready-for-review
lenhard
changed the title
|
Ok, I tried to extract the static dependency to the ProtectedTermsLoader. Doing this via method parameters is insane change propagation. It goes from the formatters to the cleanups to the BibTexParser and then you have to change practically the entire logic package. This is similar to the FileUpdateMonitor and not a feasible path of action. We don't want to insert ten parameters into every method in logic because something down in the stack might need it. Instead I did something else: The ProtectedTermsFormatter does implement the Formatter interface, but it does not fit into the Formatter framework. It is the only formatter that is dependent on external configuration and preferences. Because of this, I took it out of the framework classes and add it in the GUI, in those classes that actually need it and that have the necessary information to construct an instance of it. This results in little code changes. That's much better than propagating method parameters everywhere. This PR is again ready for review. |
lenhard
changed the title
lenhard
added
the
status: ready-for-review
| public static List<Formatter> getAvailableFormatters() { | ||
| return Collections.unmodifiableList(availableFormatters); | ||
| public static List<Formatter> getBuiltInFormatters() { | ||
| List<Formatter> availableFormatters = new ArrayList<>(); |
There was a problem hiding this comment.
Why not just new ArrayList(formatters.getAll())?
| private static final String REGEX = "regex"; | ||
|
|
||
| private static final int LENGTH_OF_REGEX_PREFIX = REGEX.length(); | ||
|
|
||
| private Formatters() { | ||
| } | ||
|
|
||
| public static final List<Formatter> getConverters() { | ||
| List<Formatter> converters = new ArrayList<>(); |
There was a problem hiding this comment.
In this case it is a defensive copy. The reference to the internal list should not escape the method, because the callers could modify it (which happens in the case of the case changers).
| @@ -45,7 +47,8 @@ public ProtectedTermsMenu(TextArea opener) { | |||
|
|
|||
| private void updateFiles() { | |||
| externalFiles.getItems().clear(); | |||
| for (ProtectedTermsList list : Globals.protectedTermsLoader.getProtectedTermsLists()) { | |||
| ProtectedTermsLoader loader = Globals.protectedTermsLoader; | |||
| for (ProtectedTermsList list : loader.getProtectedTermsLists()) { | |||
There was a problem hiding this comment.
When I get the braces right, it would make sense to directly use a stream to filter the ! isInternalList and then only iterate the result collection and apply the actions on them
There was a problem hiding this comment.
Well, the loop has side effects (adding to externalFiles), so you cannot trivially rephrase that as a lambda. Since it's also not actually the target of the PR, I would prefer not to do this change.
There was a problem hiding this comment.
That's not what I meant:
You could split that in two parts: First filter all !internal things and return the result as a Collection/List.
Second step you can iterate that result collection (a smaller result set) like it is done now.
There was a problem hiding this comment.
Ok, if you really want, there you go. Not that this matters, there is a one digit number of lists here ;-)
|
From my pov it's a go now..I think we can merge it. |
Just a code cleanup PR.
This removes the deprecated part of the
ProtectedTermsFormatterso that we have a few deprecation warnings less.