-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix for Issue #4437 - Some bugs in preference->Entry table columns #4546
Conversation
…touched are showing errors.
@rachelwu21 Thanks for your contribution! Your errors indicate that you did not setup the workspace correctly. You should run ./gradlew run once as it will create the missing folders and files |
TableRow tableRow = new TableRow(addName.getText()); | ||
addName.clear(); | ||
data.add(tableRow); | ||
tableRows.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be sufficient to add the TableRow only to the data. The TavleView should update automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good so far from my side, just a very minor thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all: Thank you for your effort so far!
However, unfortunately I have some issues with your PR.
After adding new entries to the table column they are not showing up properly, but an IndexOutOfBoundsException
is raised:
14:08:45.168 [JabRef CachedThreadPool] ERROR org.jabref.gui.util.DefaultTaskExecutor - Problem running in fx thread
java.util.concurrent.ExecutionException: java.lang.IndexOutOfBoundsException: Index: 6, Size: 6
at java.util.concurrent.FutureTask.report(FutureTask.java:122) ~[?:1.8.0_121]
at java.util.concurrent.FutureTask.get(FutureTask.java:192) ~[?:1.8.0_121]
at org.jabref.gui.util.DefaultTaskExecutor.runInJavaFXThread(DefaultTaskExecutor.java:48) ~[classes/:?]
at org.jabref.gui.importer.actions.OpenDatabaseAction.addNewDatabase(OpenDatabaseAction.java:253) ~[classes/:?]
at org.jabref.gui.importer.actions.OpenDatabaseAction.openTheFile(OpenDatabaseAction.java:225) ~[classes/:?]
at org.jabref.gui.importer.actions.OpenDatabaseAction.lambda$openFiles$2(OpenDatabaseAction.java:172) ~[classes/:?]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) [?:1.8.0_121]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) [?:1.8.0_121]
at java.lang.Thread.run(Thread.java:745) [?:1.8.0_121]
Caused by: java.lang.IndexOutOfBoundsException: Index: 6, Size: 6
at java.util.ArrayList.rangeCheck(ArrayList.java:653) ~[?:1.8.0_121]
at java.util.ArrayList.get(ArrayList.java:429) ~[?:1.8.0_121]
at org.jabref.preferences.JabRefPreferences.createColumnWidths(JabRefPreferences.java:1874) ~[classes/:?]
at org.jabref.preferences.JabRefPreferences.getColumnPreferences(JabRefPreferences.java:1888) ~[classes/:?]
at org.jabref.preferences.JabRefPreferences.getMainTablePreferences(JabRefPreferences.java:1894) ~[classes/:?]
at org.jabref.gui.BasePanelPreferences.from(BasePanelPreferences.java:35) ~[classes/:?]
at org.jabref.gui.importer.actions.OpenDatabaseAction.lambda$addNewDatabase$5(OpenDatabaseAction.java:254) ~[classes/:?]
at java.util.concurrent.FutureTask.run(FutureTask.java:266) ~[?:1.8.0_121]
at com.sun.javafx.application.PlatformImpl.lambda$null$173(PlatformImpl.java:295) ~[jfxrt.jar:?]
at java.security.AccessController.doPrivileged(Native Method) ~[?:1.8.0_121]
at com.sun.javafx.application.PlatformImpl.lambda$runLater$174(PlatformImpl.java:294) ~[jfxrt.jar:?]
at com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:95) ~[jfxrt.jar:?]
at com.sun.glass.ui.win.WinApplication._runLoop(Native Method) ~[jfxrt.jar:?]
at com.sun.glass.ui.win.WinApplication.lambda$null$148(WinApplication.java:191) ~[jfxrt.jar:?]
... 1 more
So also the values for the prefs entry "COLUMNWIDTHS" needs to be adjusted upon adding new columns (or a default value needs to be used).
Moreover, also the resizing of columns in the UI itself is not stored. So each time JabRef is opened the old table column width is used - but that was already broken before your PR I fear. @tobiasdiez @Siedlerchr Is this right?
Best regards,
Matthias
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add a default size for the new column and put in the column_width list - or the exiting column widths for all others.
Have a look at PersistanceValueStateTable
. Regarding the setting of the size, maybe you need to call setExactWidth in the TableColumnFactory
also for Normal Table Columns.
I've gotten adding column widths to work (each new column has a default column width), and I've gotten changing column widths in the UI to work. You can't decrease the column width after increasing it, but this was a problem in the master branch too. And I've noticed a problem where changing the columns in the Preferences tab doesn't change the MainTable until you restart the program. So if you change the preferences via the MainTable UI right after changing something in the Preferences Dialog, any previous changes made in TableColumnsTab is overridden. I think I can fix this if I pass the MainTable through PreferencesDialog.java to the constructor of TableColumnsTab.java. |
Hm, this is not ideal yet, it cause a massive performance break and maybe it is related to the |
…-entry-table-cols
So far I have:
Mostly what I did for the last few commits was make sure changing the columns both from Preferences and from PersistenceVisualStateTable doesn't cause a java.lang.IndexOutOfBoundsException. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks already quite good to me. Please fix the few remaining suggestions (also from the other reviewers) and then we can merge. Thanks!
|
||
} | ||
|
||
private void onColumnWidthChange() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method appears to be identical to updateColumnPreferences
below, or do I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onColumnWidthChange()
was redundant and a brain fart on my part and I've removed it. Thanks for the head up. :)
}); | ||
|
||
tableRows.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you understand the purpose of the tableRows
field? It appears that it is always synced to the data
collection anyway. If that's the case, could you please remove tableRows
. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look and it turned out it was redundant with data
as you said. I've removed it.
if (colSetup.getFocusModel() != null && colSetup.getFocusModel().getFocusedIndex() != -1) { | ||
tableChanged = true; | ||
int row = colSetup.getFocusModel().getFocusedIndex(); | ||
TableRow tableRow = data.get(row); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it should also be sufficient to just change data
without any further updates to colSetup
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
@@ -502,11 +478,11 @@ public void actionPerformed(ActionEvent e) { | |||
try { | |||
String name = panel.getMainTable().getColumnName(i).toLowerCase(Locale.ROOT); | |||
int width = colMod.getColumn(i).getWidth(); | |||
if ((i <= tableRows.size()) && ((String) colSetup.getValueAt(i, 0)).equalsIgnoreCase(name)) { | |||
if ((i <= data.size()) && ((String) colSetup.getValueAt(i, 0)).equalsIgnoreCase(name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole class "UpdateWidhtsAction" is now superflous, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I got rid of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rachelwu21 can you push these changes to github, so that we can merge? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I'm so sorry! I've pushed it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! Thanks!
Hey guys! I just wanted to thank you for all the advice and help you've given me, and for labeling good first-timer issues. I found this a lot of fun. |
You're welcome! We have to thank you for your contribution 🥇 and hope you are interested in contributing some more 😄 ! Feel free to ask if you need any advice/help for an issue |
* upstream/master: (583 commits) update jfoenix and gradle plugins Replace outdated transformer log4j2 with official new one Update journalList.txt Fix for Issue #4437 - Some bugs in preference->Entry table columns (#4546) Don't set column sort type at startup (#4577) Add uncaught exception message (#4565) Converts integrity check dialog to JavaFX (#4559) Do not extract file ending from Urls (#4547) Bump checkstyle from 8.15 to 8.16 (#4562) Bump xmpbox from 2.0.12 to 2.0.13 (#4561) Delete the deprecated BibEntry Constructor (#4560) Refactor BibEntry deprecated method (#4554) Added extra stats to be sent with MrDLib recommendations (#4452) improve styling of preferences side menu (#4556) Cleanup interfaces (#4553) Bump fontbox from 2.0.12 to 2.0.13 (#4552) Bump pdfbox from 2.0.12 to 2.0.13 (#4551) Bump wiremock from 2.19.0 to 2.20.0 (#4550) Fixes that renaming a group did not change the group name in the interface (#4549) Bump applicationinsights-logging-log4j2 from 2.2.1 to 2.3.0 (#4540) Bump antlr4-runtime from 4.7.1 to 4.7.2 (#4542) ... # Conflicts: # src/main/java/org/jabref/gui/JabRefFrame.java # src/main/java/org/jabref/model/entry/BibtexString.java
* upstream/master: (229 commits) Try to fix not on FX thread for search and autocomplete (#4618) Convert DuplicateResolverDialog to javafx (#4601) Fix for BibTex source tab parsing issue if field contains {} (#4581) Convert OO/LO SidePanel to javafx (#4341) Convert "Customize importer" dialog to JavaFX (#4608) Convert "From Aux file" dialog to JavaFX (#4607) Convert "Show preferences" dialog to JavaFX (#4605) Fix not on FX thread exception Force javafx to run thread (#4604) Convert new version dialog to JavaFX (#4602) Add a variable to track the change in preview style (#4587) Solution for submitting dialog with Ctrl + Enter (#4496) (#4592) Bump mysql-connector-java from 8.0.13 to 8.0.14 (#4599) Fix overlapping font in id entry type (#4595) update jfoenix and gradle plugins Replace outdated transformer log4j2 with official new one Update journalList.txt Fix for Issue #4437 - Some bugs in preference->Entry table columns (#4546) Don't set column sort type at startup (#4577) Add uncaught exception message (#4565) Converts integrity check dialog to JavaFX (#4559) ... # Conflicts: # build.gradle # src/main/java/org/jabref/gui/FindUnlinkedFilesDialog.java # src/main/java/org/jabref/gui/JabRefFrame.java # src/main/java/org/jabref/gui/fieldeditors/EditorTextArea.java # src/main/java/org/jabref/gui/fieldeditors/EditorTextField.java # src/main/java/org/jabref/gui/openoffice/CitationManager.java # src/main/java/org/jabref/gui/openoffice/OOBibBase.java # src/main/java/org/jabref/gui/openoffice/OpenOfficePanel.java # src/main/java/org/jabref/gui/openoffice/OpenOfficeSidePanel.java
I'm trying to fix issue #4437 , and I think I've got it. The 'Column width' stuff have been removed and the 'Field name' is now editable. You need to close and reopen the app for the preference changes to take effect.
Edit: It used to be that some files I haven't touched were showing errors, so I can't build the project to see if my changes actually fixed anything. But that's not a problem anymore.
org.jabref.logic.importer.fileformat.EndXmlImporter.java has errors because the entire org.jabref.logic.importer.fileformat.endnote folder is missing.
(and so on)