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 all 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
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.
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.
Isn't abbreviation repository is already kinda a
Map
? Isn't there a method for getting abbreviations/unabbreviations from thereThere 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 is what I talked about in the previous comment. Doesn't abbreviation repository doesn't have some kind of
get
method?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 think this should be moved to
JournalAbbreviationPreferences
. This is not related to logic inAbbreviate
Action, and it's quite generalThere 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?