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

Consistency check for text fields #11939

Closed
koppor opened this issue Oct 12, 2024 · 7 comments · Fixed by #12057
Closed

Consistency check for text fields #11939

koppor opened this issue Oct 12, 2024 · 7 comments · Fixed by #12057
Assignees
Labels
component: entry-editor dev: code-quality Issues related to code or architecture decisions FirstTimeCodeContribution Triggers GitHub Greeter Workflow

Comments

@koppor
Copy link
Member

koppor commented Oct 12, 2024

Follow-up to #11886.

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))

@ShunL12324
Copy link
Contributor

Hi, I’d like to try working on this issue—could you please assign it to me?

@koppor koppor moved this from Free to take to Assigned in Candidates for University Projects Oct 13, 2024
@koppor koppor added the FirstTimeCodeContribution Triggers GitHub Greeter Workflow label Oct 13, 2024
@koppor
Copy link
Member Author

koppor commented Oct 13, 2024

@ShunL12324 was first - sorry @lllllllittlesun

Copy link
Contributor

Welcome to the vibrant world of open-source development with JabRef!

Newcomers, we're excited to have you on board. Start by exploring our Contributing guidelines, and don't forget to check out our workspace setup guidelines to get started smoothly.

In case you encounter failing tests during development, please check our developer FAQs!

Having any questions or issues? Feel free to ask here on GitHub. Need help setting up your local workspace? Join the conversation on JabRef's Gitter chat. And don't hesitate to open a (draft) pull request early on to show the direction it is heading towards. This way, you will receive valuable feedback.

Happy coding! 🚀

@koppor
Copy link
Member Author

koppor commented Oct 13, 2024

Remember this is a rather hard one isse!

@ShunL12324
Copy link
Contributor

The current implementation makes this JUnit test difficult. The check for "whether all instances of X receive a field with matching properties (X instanceof TextArea <=> fieldProperties.contains(MULTILINE_TEXT))" cannot be fully ensured. This is because fieldProperties.contains(MULTILINE_TEXT) can only be determined at runtime, unless we analyze each class creation to confirm if the field parameter includes the MULTILINE_TEXT property. Furthermore, in many FieldEditor implementations, multiline checks are primarily handled using the final boolean isMultiLine rather than relying on field properties.

A potentially better approach might be to first check if the class includes the field private final TextInputControl textInput. Then, using a tool like JavaParser, analyze whether the class performs a MULTILINE_TEXT property check, and finally, confirm if EditorTextArea instances are being created.

Does this sound like the right direction?

@koppor
Copy link
Member Author

koppor commented Oct 19, 2024

I quickly checked. The names are different - maybe check for class only

Image

TBH, I do not know how to check.

The class to check is org.jabref.gui.fieldeditors.FieldEditors. Maybe, only the if block at org.jabref.gui.fieldeditors.FieldEditors#getForField needs to be analyzhed with JavaParser.


Maybe, it is enough to add some JavaDoc to org.jabref.model.entry.field.FieldProperty#MULTILINE_TEXT and org.jabref.gui.fieldeditors.FieldEditors#getForField - and add entry-editor.md to docs/code-howtos and sketch how it works with field creation.

@ShunL12324
Copy link
Contributor

I also tried to analyze every class that implements FieldEditorFX, but the difficulties remain the same. We can neither determine if the class will or should have FieldProperty.MULTILINE_TEXT nor if the class actually checks this property to decide whether to use TextArea or TextField. In fact, FieldProperty.MULTILINE_TEXT is not being used. Maybe we should just add some comments and documentation at this moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: entry-editor dev: code-quality Issues related to code or architecture decisions FirstTimeCodeContribution Triggers GitHub Greeter Workflow
Projects
Development

Successfully merging a pull request may close this issue.

2 participants