Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add toggle functionality for journal abbreviation lists #12912
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
base: main
Are you sure you want to change the base?
Add toggle functionality for journal abbreviation lists #12912
Changes from 49 commits
37c8d2c
1a5edb3
ed985ec
5828a7e
2eee019
623ab21
0589d14
62ccc81
b6a92b8
774f5fb
ed58605
00b8d24
e3b62b7
d3f7ff4
230c70f
35900b1
fd4544d
d9e6b20
fe60693
7ba2574
8fc0d82
10491f5
d1e73e0
39c822d
421b08f
305beee
df1553c
a68cc84
b1237af
09999db
2f9fc82
19cf34b
9dafa2a
9b4a43f
d555a09
c85ec97
5160d78
ccea652
a57193f
818436d
26fc774
153c8ad
2a6604e
a23d3c8
d9437e2
c15e554
f50883d
7247038
19d1401
50c3549
bd53428
9c3d8cc
0cf60ea
eb07892
c8b9b51
f58543b
3a40825
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
JournalAbbreviationRepositoryManager
-- I don't like this approach.Typically, in JabRef for dependencies, we do this:
WebView
s) we use static classes.So what I meant to say that we don't use static/singleton classes in a raw form like this. I'm not sure, why this approach was taken, what was the problem with previous approach where a repository was taken from constructor?
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.
Thank you for your feedback @InAnYan, this approach was wrongly taken by me yesterday night as I was rushing to get it done (I will take note to have a calm mind when pushing for code changes). Now I understand that this approach is wrong. I cc-ed you in a comment above for details regarding what was the problem to solve, what was the initial approach taken and why this approach was subsequently used by me.
In short, the original problem I was trying to solve was the inefficient creation of new repositories on every operation:
Then, I implemented the separate manager class that works functionally, but I now see now it adds unnecessary abstraction.
I will make the following changes:
JournalAbbreviationRepository
getInstance(preferences)
method thereJournalAbbreviationRepository
This way, the repository itself will handle when to reload based on preference changes, without needing extra layers.
May I ask whether this approach sound right? I appreciate the guidance and am eager to improve the implementation. :)
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.
@InAnYan I think I understand the issues better.
First of all, creating a separate manager was unnecessary and did not align with JabRef's architectural style.
I have completely removed the manager class and implemented a more localized solution to address the efficiency problem. Instead of creating a new repository for every abbreviation operation, I have added instance-level caching in the
AbbreviateAction
class. This approach stores the last used preferences and repository as instance fields, implements apreferencesChanged()
method to compare current vs. cached preferences and only creates a new repository when preferences actually change. The changes can be seen in 50c3549.I think this gives us the performance benefits without introducing a singleton pattern or static caching. The repository itself still handles filtering for enabled/disabled sources through its specialized methods.
Please let me know if this approach looks good to you or if you would like me to make further adjustments. Thank you so much!
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.
Why do you collect a
Map
instead of just usingfreshRepository.isSourceEnabled()
?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 yes,
isSourceEnabled(sourceKey)
method already does the same thing. I just made the changes in 9c3d8cc :)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 seen that you removed line https://github.com/JabRef/jabref/pull/12912/files#diff-3fa252360fc27ea81a62af5e94c9a8263b2519ca8845cd50c55d0a001f334e41L123 -- is this class still used anywhere?
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, but this class is still being used.
I have simplified the filtering logic to determine which entries to process, but the actual unabbreviation is still performed by
UndoableUnabbreviator
at lines 170-174 inAbbreviateAction.java
:There are also tests that verify its functionality.
This class is responsible for the actual unabbreviation and undo operations, while my changes simplified how we determine which entries to process. The class is still essential for the functionality to work correctly.
May I ask whether this sounds right?