Convert some fields to TextArea #11886
Conversation
There was a problem hiding this comment.
Nice work.
Small comments inside.
Optional follow-ups:
- Tab in the last text field in a tab should focus the next tab in the tabs of the entry editor.
- Tab in empty text area field should focus the next field
- Consistency check
Consistency check:
There should be an automatic checker (JUnit) test case. JabRef has org.jabref.model.entry.field.FieldProperty#MULTILINE_TEXT. All of these fields should be offered as TextArea, all others (!) as TextField
I think, this is a hard one, but much to learn.
The "only" way is maybe to use Refaster template recipes and check if OpenRewrite would change anything.
Maybe, this also does not work and one would need to use JavaParser to check the constructor at org.jabref.gui.fieldeditors.FieldEditorFX, whether all new X get a field passed matching the X (X instanceof TextArea <=> fieldProperties.contains(MULTILINE_TEXT))
If you can do any of these, I would be happy. Otherwise, I will open follow-up issues. Initally, all of these ideas look hard to implement.
Siedlerchr
added
the
status: changes required
I'd like to do the first one please (Tab in the last text field in a tab should focus the next tab in the tabs of the entry editor). One of my friend @yeonissa would like to be assigned with the second one. We are doing this as part of university course in ANU. Also, I've already made the changes you requested. Let me know if you need anything else. |
Do you need a seperate issue for that be created?
Do you need a seperate issue for that be created? |
koppor
removed
the
status: changes required
|
The changes need a second review and thus this PR awaits a second review. |
|
@koppor yes please - can you make separate issues for that? Thanks a lot. |
| private PasteActionHandler additionalPasteActionHandler = () -> { | ||
| // No additional paste behavior | ||
| }; |
There was a problem hiding this comment.
You don't need to create an interface; you can use java.lang.Runnable instead. Please also update in org.jabref.gui.fieldeditors.EditorTextArea, or remove it if it's not being used.
There was a problem hiding this comment.
i think using Runnable here would be semantically inappropriate, since it's typically used for thread-related tasks. Sticking with a more specific interface or lambda for paste handling keeps the intent clearer.
There was a problem hiding this comment.
About Runnables. I also thought it's only for threading, but no.
It's a general and accepted way to have a function type that accepts to arguments and returns no type
There was a problem hiding this comment.
i think using Runnable here would be semantically inappropriate, since it's typically used for thread-related tasks. Sticking with a more specific interface or lambda for paste handling keeps the intent clearer.
Reading the documentation for Runnable, I don’t see any threading-related tasks mentioned.
The interface is similar to how you implemented PasteActionHandler. (https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Runnable.java).
There was a problem hiding this comment.
This is a general code style question. Does one want to have strong typing or not. - Seeing that the setting of this is very generic, I also don't see the need for an additional class type.
() -> textField.setText(new CleanupUrlFormatter().format(new TrimWhitespaceFormatter().format(textField.getText())))
Just changing to Runnable should fix it?
Regarding the intention of runnable:
Represents an operation that does not return a result.
This is excactly the thing we are looking for here, isn't it?
Source of Runnable:
package java.lang;
/**
* Represents an operation that does not return a result.
*
* <p> This is a {@linkplain java.util.function functional interface}
* whose functional method is {@link #run()}.
*
* @author Arthur van Hoff
* @see java.util.concurrent.Callable
* @since 1.0
*/
@FunctionalInterface
public interface Runnable {
/**
* Runs this operation.
*/
void run();
}There was a problem hiding this comment.
I changed it to Runnable in my latest commit. Thanks.
| return menuItems; | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * The default context menu with a specific menu item to cleanup URL. | ||
| * | ||
| * @param textArea text-area that this menu will be connected to | ||
| * @param textField text-area that this menu will be connected to |
There was a problem hiding this comment.
text-area there in the doc string. Should be text-field, I think
src/main/java/org/jabref/gui/fieldeditors/contextmenu/EditorMenus.java
Outdated
Show resolved
Hide resolved
#11937 and #11938. @juliusalberto Please comment there so that I can assign you (or your colleague) |
| private PasteActionHandler additionalPasteActionHandler = () -> { | ||
| // No additional paste behavior | ||
| }; |
There was a problem hiding this comment.
This is a general code style question. Does one want to have strong typing or not. - Seeing that the setting of this is very generic, I also don't see the need for an additional class type.
() -> textField.setText(new CleanupUrlFormatter().format(new TrimWhitespaceFormatter().format(textField.getText())))
Just changing to Runnable should fix it?
Regarding the intention of runnable:
Represents an operation that does not return a result.
This is excactly the thing we are looking for here, isn't it?
Source of Runnable:
package java.lang;
/**
* Represents an operation that does not return a result.
*
* <p> This is a {@linkplain java.util.function functional interface}
* whose functional method is {@link #run()}.
*
* @author Arthur van Hoff
* @see java.util.concurrent.Callable
* @since 1.0
*/
@FunctionalInterface
public interface Runnable {
/**
* Runs this operation.
*/
void run();
}
I have changed some fields from TextArea to TextField in some fields: URL, identifier, owner, and ISSN.
Closes #11785
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)