Replaced DefaultFormBuilder with FormBuilder#205
Conversation
|
Well, looks like a decent mess of commits... Still trying to learn how to merge/rebase etc. Tried rebase -i now and for a while I thought that might work, but then at the end even more of a mess (and that is not pushed)... |
|
OK, got it down a decent amount of commits (from 50+), so now I somewhat happy at least. Of course I can reduce it further (I guess, considering that I succeeded with this). |
|
Yes, I saw that the translation commands were inconsistent and it appears that both works. |
|
Oh, the new See e.g. changes in |
|
Regarding I believe that the improvement of |
There was a problem hiding this comment.
We have a script that automatically extracts the used language keys. This only works when we do not pass a variable to the lang method. Can you please inline the language keys as Strings?
There was a problem hiding this comment.
Some lines below, the same strings are inlined. Therefore, a comment on the strings a few lines above is enough. (I hope to remember the setting correctly, currently on the road)
There was a problem hiding this comment.
I'll sort this out. I have a better idea of the translation process now.
The reason for using variables in this way (in general, as it is not really the case implemented here I realize) would be to only need to change the original string in one position, if you for some reason do want to change it. For translations there is obviously only going to be one string to translate anyway, but for consistency it can make sense to do it like this.
I assume it may be better to do something like
private final String editTitle = Localization.lang("Edit file type");
private final String newTitle = Localization.lang("New file type");
...
String title = editTitle;
if(entry.getName().isEmpty()) {
title = newTitle;
}and so on?
Regarding @koppor:s comment for lines 215-219, I'll look into the actual operation again. I had the impression that one can use the same dialog instance if you click on another item, but not sure if that was the case and that I forgot to change back. If that's not the case, I assume
String title = Localization.lang("Edit file type");
if(entry.getName().isEmpty()) {
title = Localization.lang("New file type");
}is a slightly better solution? (Assuming that you more commonly edit than add file types.)
Will continue working on this PR tonight.
|
OK! Reduced the number of commits. Not sure if it was a good idea or not. Kept one as there were a few comments related to that, but it didn't work the way I expected... The last commit contains the changes introduced now. Mainly the ones that were requested (and a few other minor things). |
|
You don't always need to reduce the commits. When something is reviewed by us, new changes can simpler be evaluated as diff than starting from scratch 😇 |
|
Well, the idea was at least one of the previous commits which were commented on, but somehow that didn't work out. Anyway, all diffs are in 6433826, but clearly you can not see your previous comments... Anyway, I think things are improving on my side, so in the future you can expect PRs with single commits and then additional commits in response to comments. :-) |
|
👍 The discussed abstract class and other things (like |
|
Yes, I plan to do that (CITE_COMMAND, abstract class) in the near future, but I believe a separate PR is more appropriate to keep the changes well separated (although I admit I did some changes in this PR such as unified strings which is unrelated to stated purpose...). |
Replaced DefaultFormBuilder with FormBuilder
Replaced the deprecated DefaultFormBuilder with FormBuilder in about ten instances.
As I was working with the external programs code, the strings there were unified to ease the translation work a bit.