WIP: Improve reversibility #7469
Conversation
|
Hi @antalk2, thanks so far for your efforts you put in this PR. |
Also: extract common part: insertNamedTextContent
Problem was: - Turn Settings/Automatically sync.. off on OO/LO panel - Select an article, Click Cite - The reference mark created was empty. It turned out, that in OOBibBase.insertReferenceMark position.setString(citText); failed to set the text. Solution was: create a new cursor and use that.
…ception in update.setOnAction
|
At the commit The code should work and conform to the style checker. There are some known bugs, at this point in TODO comments, Currently I am trying to solve some of the bugs and separate Questions about the Bibliography
The questions are:
I would choose bookmark now, (with maybe giving an option
Citations: Reference marks or bookmarks (or something else)A weak point of reference marks marking citations is that in LO On the other hand bookmarks survive (the copy gets a new name with a Comments and invisible markup also survive, but probably take more time Was: Merge losing connection to pageInfo
This seems doable, even with some level backward compatibility What I am worried about is: if we repeatedly change the representation, When should I start a new branch?At the commit mentioned above: we have the code reorganized, mostly Shall I start a new branch somewhere about there? Other: OOBibBase.org is probably not where you would want it to be in |
|
Can you please stop? This PR is totally loosing focus on the issue. It seems that you refactor files that are not directly linked to the original issue. It will not be possible to review this PR anymore. |
There was a problem hiding this comment.
This is a hard one. Some first, minimal comments.
We have some "minimum coding guidelines".
Could you please re-iterate on https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace?
It seems that the IDE used was not configured. Thus, checkstyle will fail.
| * pageInfo for this citation, provided by the user. | ||
| * May be null, for none. | ||
| */ | ||
| String getPageInfoOrNull(); |
There was a problem hiding this comment.
Do not return null. Use Optional.
| @@ -16,14 +16,17 @@ | |||
|
|
|||
| public class StyleLoader { | |||
|
|
|||
| public static final String DEFAULT_AUTHORYEAR_STYLE_PATH = "/resource/openoffice/default_authoryear.jstyle"; | |||
| public static final String DEFAULT_NUMERICAL_STYLE_PATH = "/resource/openoffice/default_numerical.jstyle"; | |||
| public static final String DEFAULT_AUTHORYEAR_STYLE_PATH = | |||
There was a problem hiding this comment.
It would be nice if you reformated changed code only.
There was a problem hiding this comment.
I agree.
The question of long lines comes back repeatedly.
I have 106 characters of horizontal space (about half width of the screen), and tend to break lines that are longer.
Idea would provide at most 183 (full screen, side bars closed), unless I find a smaller font.
The style rules seem to prefer long lines, and indeed, there are many above 100,
some even above 300. I was wondering how people manage to read those.
Are "fit-to-width" PRs something like the project would consider?
If yes, what would be a targeted limit on line length?
This may also raise: where to break.
There was a problem hiding this comment.
I was wondering how people manage to read those.
I scroll 😛
Are you asking about 'soft-wrap'? Screen-shot is from Intellij
There was a problem hiding this comment.
It is possible. Breaking at white space instead of at the character at the edge of the window
helps somewhat, but still destroys the "visible indentation suggests code structure" effect.
My guess was they do no split their windows vertically, as Idea and probably other
IDEs put compilation results at the bottom under a horizontal split.
There was a problem hiding this comment.
"visible indentation suggests code structure" effect.
I am not sure what you are referring to, first of all
- are you using Intellij?
- does the below settings help?
they do no split their windows vertically, as Idea
"Idea" = IntelliJ IDEA? I split mine both horizontally and vertically o.O
There was a problem hiding this comment.
- are you using Intellij?
No. Not to edit code.
- does the below settings help?
I am not sure what you are referring to, first of all
It seems you guessed correctly.
These settings improve the results by not destroying the outline.
This makes it more understandable why Intellij IDEA users seem to care less
about keeping a limit on their line lengths.
What it seems to miss when compared to hand-edited
breaks is a preference to break at higher points in the parse tree.
For example breaking between arguments of the outer call, not within.
There was a problem hiding this comment.
For example breaking between arguments of the outer call, not within.
Indeed. I don't know of any options for this within Intellij.
| encoding); | ||
|
|
||
| OOBibStyleParser.ParseLog parseLog = newStyle.getParseLog(); | ||
| if ( parseLog == null ) { |
There was a problem hiding this comment.
It seems that the IDE used was not configured according to https://devdocs.jabref.org/getting-into-the-code/guidelines-for-setting-up-a-local-workspace. Thus, checkstyle will fail.
There was a problem hiding this comment.
Does each commit have to pass checkstyle?
I hoped it is enough to check from time to time, and before PRs.
There was a problem hiding this comment.
Each commit doesn't have to pass Checkstyle, but it becomes easier to review if they pass or are close to passing Checkstyle. Is there anything except the line-wrapping issue that discourages you from using the default settings?
There was a problem hiding this comment.
Is there anything except the line-wrapping issue that discourages
you from using the default settings?
I assume "default settings" above refers to "checkstyle with JabRef
settings". I am using those now, through a call to checkstyle with JabRef
settings in my Makefile before "gradlew build". Earlier I was relying
on visits to Intellij IDEA to check from time to time, and by regularly
postponing this I collected many non-confirming spaces.
the line-wrapping issue
This appeared in two situations.
One is when I formatted code in order to read it.
Now I removed many (all?) of those.
I understand it is not a good idea to mix formatting with actual changes,
and often separated these into "format" and "fit-to-width" commits.
But these were still interspersed with actual changes.
This is why I asked about "fit-to-width" PRs: these would separate
code formatting and code changes. So far my impression is that
these are considered unnecessary among the people here,
probably because -- as you have shown above -- it is less of a problem
for Intellij IDEA users. Unfortunately I am not ready to switch to it.
The other situation is with new or changed code:
class X {
public static //x
ALongDescriptiveNameOfATemplate<ALongParameter, AndOfcourseAnAnotherOne> //x
descriptiveMethodName( //x
LongParameterType firstParameter,
Another secondParameter) {
}
}
my impression is that none of the line breaks marked with //x are
considered OK (even if checkstyle accepts some of them). Taken together,
they can result in long lines, like this:
class X {
public static ALongDescriptiveNameOfATemplate<ALongParameter, AndOfcourseAnAnotherOne> descriptiveMethodName(LongParameterType firstParameter,
Another secondParameter) {
}
}
On the question of independent PRs: although I could point to some
changes that could be considered for thematic splitting, these are small when
compared to a large body of restructuring. (I removed changes to the *.jstyle parser
as well as changes outside openoffice directories (except JabRef_en.properties)).
GUI changes: there are no changes to the OO Panel or other parts of
the normally visible GUI.
Changes to gui-related code consist so far basically of
-
(g1) replacing series of calls to OOBibBase by a single call (moving
detailed knowledge of OOBibBase working from GUI stuff to an
OOBibBase method dedicated to the given GUI function) -
(g2) catching more exceptions (with associated display of warning
dialogs) -
(g3) checking more preconditions (with associated display of warning
dialogs).
OO interface changes: What is changed as seen by the user in Libreoffice?
-
(o1) unresolved citations are not thrown away, they get an
"Unresolved(citationKey)" representation in the text (unless using
numbered citations) and they also get a bibliography entry with a
similar content -
(o2) these bibliography entries now contain clickable
cross-references pointing to the corresponding citation groups in
the text. -
(o3) GUI actions now wrapped into OO Undo groups, so their effects
can be undone with a single click (except those affecting pageInfo in
custom properties) -
(o4) parts of GUI actions are wrapped into "disable screen
refresh" which reduces flashing. The exception is the part
deciding "visual order", which relies on asking the position of the
text view cursor. -
(o5) more careful sorting of footnote markers now allows Merge to
more reliably discover consecutive citation groups in footnotes. -
(o6) citation groups with no intervening space can now be merged
-
(o7) Even with Settings/Automatically sync.. off, leave recognizable marks
Problem was:
- Turn Settings/Automatically sync.. off on OO/LO panel
- Select an article, Click Cite
- The reference mark created was empty.
It turned out, that in OOBibBase.insertReferenceMark
position.setString(citText); failed to set the text.Solution was: create a new cursor and use that.
OOBibStyle changes:
-
(j1) (Removed parser changes. Note: the parser does need changes:
silently ignoring user input the parser does not understand, providing
no warning on mistyped keys, like for example
BibtexKeyCitations (as appears in the official exemples:
jabref/src/main/resources/resource/openoffice/default_authoryear.jstyle,
jstyles.jabref.org-master/turabian-english.jstyle,
jstyles.jabref.org-master/turabian-deutsch.jstyle
and jstyles.jabref.org-master/remoteSensingOfEnvironment.jstyle)
instead of the expected BibTeXKeyCitations,
inheriting settings from a previous parse, ignoring changes if
they only affect the LAYOUT section make it a nightmare
figuring out why some changes to the style have no effect) -
(j2) xxxMarkupBefore xxxMarkupAfter
*.jstyle now allows insertion of markup before and after a citation group
as well as some of its partsimplemented
CitationGroupMarkupBefore and CitationGroupMarkupAfter
AuthorsPartMarkupBefore and AuthorsPartMarkupAfter
AuthorNamesListMarkupBefore and AuthorNamesListMarkupAfter
AuthorNameMarkupBefore and AuthorNameMarkupAfterThis was an opportunistic extension, allowed by "Most text we emit
into the document handled as OOFormattedText". The latter was needed
for proper handling of italicization of "et al.", that is doing it
(using markup) when creating the citation marker, not heuristically
by search-and-edit EtAlString when inserting.
Of these
- (g1) and (g2) are part of the restructuring.
- (g3) and (o3) were implemented to help during testing-by-hand
- (o4) was not necessary for testing. It just follows a similar pattern as
the Undo part and I happened to find the corresponding API in the OO docs. - (o5) In principle could have been left out. The hypothesis about
what could be the problem (why citation groups in footnotes are not
merged) of course came from the effort to understand what does the
code do. I could have left it as a hypothesis with some notes on
what do I think, and why, and which parts of the code should be
changed to attempt a solution. And after the extra effort to create
these notes, then let's say a month later, I could scratch my head what was I
thinking and where are those parts now. As time goes, I do forget things,
so it is time to locate and reread the corresponding code ... Doing things
the administratively appropriate way, while it has its virtues,
may result in losing the opportunity to do them at al.
There was a problem hiding this comment.
I assume "default settings" above refers to "checkstyle with JabRef settings".
Yup, sorry about the confusion.
So far my impression is that these are considered unnecessary among the people here,
You are right.
Taken together, they can result in long lines, like this:
Unfortunately, I'd suggest going with the long lines even if they aren't always enforced by Checkstyle.
This is why I asked about "fit-to-width" PRs: these would separate code formatting and code changes.
In practice, when reviewing a PR I'd be using the Files changed tab in the PR and it easily becomes "cluttered" if I happen to look at a commit that is "fit-to-width".
On the question of independent PRs: although I could point to some changes that could be considered for thematic splitting, these are small when compared to a large body of restructuring.
Can they be split without the larger body of restructuring? Even (g1), (g2), (g3), and (o3) seem like a large PR in itself but it would allow us to better understand what you are changing and why in a more incremental way. Frankly, even (g2) by itself could be considered a good contribution.
There was a problem hiding this comment.
The reason why CitationEntry is in logic is, is just simply the fact that nobody moved it to a specific model package.
There is simply no org.jabref.model.openoffice.
The name itself is fine.
Regarding UNO:
https://api.libreoffice.org/ (see examples and IDL apidoc)
I had spent some time with the JabRef OO stuff and also the UNO stuff in general (some LO macros)
And https://wiki.openoffice.org/wiki/Documentation/DevGuide/OpenOffice.org_Developers_Guide
Although it's from OO the general concepts still apply to LO and the usage in JabRef. You can skip the part of the connection to LO because that's a topic on its own which is not relevant.
There was a problem hiding this comment.
Options? Email?
An up to date PR but expect the first comments to be about adhering to the codestyle, e.g., try not to abbreviate names. It is to make it easier to understand and review.
There was a problem hiding this comment.
like separating oostyle
I don't feel that I understand your goal well enough. Perhaps it will become clearer what class belongs where soon (it is not necessarily always clear-cut).
How they relate to "Catch and wrap all API exceptions". Where should we do that?
...
Is threading up everything
in distinct exception types a good thing?
It depends. Many of those exceptions seem to indicate that there is an exception that will prevent what the user is trying to do from happening and therefore should show an error (i.e., it isn't enough with logging it and doing something else).
I'll try to keep an eye out for good examples.
Or should we collect them to some more generic types derived from JabRefException?
Mostly not, but there are some examples of where it's done, ShortDOIServiceException, FetcherException, etc.
In one reading all is well, OOPanel catches (tries to catch) everything, thus JabRef proper
is not exposed to these. In another reading it is all wrong.
I'd guess that is good enough for most of the exceptions, at least for now.
There was a problem hiding this comment.
on CitationEntry
Context
You are not missing anything. It is not intended to be private to
openoffice. You are expressing the class' concern/responsibility
(logic) and use (openoffice) already. Other parts of JabRef can depend
on logic/openoffice if it is needed in the future.https://devdocs.jabref.org/getting-into-the-code/high-level-documentation
The model represents the most important data structures (BibDatases, BibEntries, Events, and related aspects) and has only a little bit of logic attached. The logic is responsible for reading/writing/importing/exporting and manipulating the model, and it is structured often as an API the gui can call and useLooking at it I am not sure why CitationEntry is in logic/openoffice instead of model/openoffice.
For example, CitationEntry, with its fairly generic name was in logic/openoffice (now in logic/oostyle (? this looks wrong, I'll move it back)). Its sole purpose is to pass some data fromWhat else would you call it?
It is imported asimport org.jabref.logic.openoffice.CitationEntry;
which, in my opinion, makes it quite specific even if the class name is generic.
The reason why CitationEntry is in logic is, is just simply the fact that nobody moved it to a specific model package.
There is simply noorg.jabref.model.openoffice.
The name itself is fine.
Notes
It is not intended to be private to openoffice.
Other parts of JabRef can depend on logic/openoffice if it is needed in the future.
The model represents the most important data structures
What else would you call it?
Let's review what is it, and what is it used for?
What is it
public class CitationEntry implements Comparable<CitationEntry> {
private final String refMarkName;
private final Optional<String> pageInfo;
private final String context;
[...] // getters, setters, Comparable<CitationEntry> implementation based on refMarkName
}
-
refMarkName: on return from the gui allows to find the citation group the pageInfo belongs to.
In the near future it needs to be extended to refer to citations, since pageInfo will be associated
to citations, not citation groups. A bit further in the future (not part of the current changes)
reference marks may be replaced with bookmarks, or possibly Comments which can also be listed
in the LibreOffice Navigator panel, but (I think) do not have names, or, to promote
interoperability and easy editing they may be replaced
with a textual representtaion similar to what JabRefConverter does (also nameless).To minimize changes I kept refMarkName here, but it basically stands for CitationGroupID.
To refer to citations CitationPath is used elsewhere.As long as we need to handle old-style (pageInfo for citationGroups) documents
in ManageCitationsDialog we need to support both variations. -
pageInfo: the only thing that ManageCitationsDialog allows to change -
context: a piece of text from the the document: the text of the citation marker
with up to 30 characters on each side. Changing to pageInfo-for-citations will need
a change here: "text-before [1-7] text-after" does not tell the user to which of the 7 sources
does this pageInfo belong to. This also applies to Author-year citations.
Based on this, I would argue that
- CitationEntry is not in any sense among the "the most important data structures"
- I do not see a future where any part of JabRef outside openoffice, or indeed, any part
inside openoffice apart from those implementing
ManageCitationsDialog would want to use this class.
It represents a collection of data for a very particular purpose.
On naming: the only thing that ManageCitationsDialogView allows is:
edit pageInfo records. It could possibly be extended with a
"link-to-location-in-the-document", and sorting (by cited source / by
document order). To do more (what actually?) would probably require
further dialogs, or at least a major revision of the current GUI.
I am not even sure that ManageCitationsDialog (or, to reflect what
it does, "PageInfoEditor") is the right direction. Both Jabref and
LibreOffice GUIs are built with a "the whole screen is mine" mindset,
(even the 2nd demo gif here
struggles showing them together).
It is probably best if we try to minimize the number of required
switches between them. Although ManageCitationsDialog is a dialog with its
own window, so it might fit on the same screen as LibreOffice,
its "parallel" use with LibreOffice is limited,
because it shows a snapshot of the document: if the user edits the text (he
cannot do much, but may remove some citation groups), the state in
ManageCitationsDialogView becomes outdated (it is unavoidable, unless
we can hook into LibreOffice to "let me know when this reference mark
is deleted", or start polling). If textual representation (similar to
JabRefConverter's, but extended to include pageInfo) comes around, the
PageInfoEditor functionality may probably better served by that: it
would make moving or copying citations between locations as well as
editing the pageInfo parts a more natural experience for
users of LibreOffice, as opposed to opening extra dialogs from an external
program (JabRef) for these purposes.
Clickable links to the list of citation groups (reference marks) are already provided
by the LibreOffice Navigator (we could add bookmarks for the discovered textual parts, too).
We could add links from the bibliography entries
to the citation groups containing citations of a given source (an exploratory version
of this is in the code, only shown for Unresolved sources to stay close to the earlier
output in the normal case).
Summary
-
CitationEntry is unlikely to be of interest outside its current use
(passing around a collection of data collected for ManageCitationsDialog) -
Depending on the future of ManageCitationsDialog
it may need modifications to serve the extended purpose or may become
unused if in-LibreOffice solutions make ManageCitationsDialog unnecessary. -
Naming CitationEntry: something like ManageCitationsDialogEntryData.
(there is also "CitationEntryViewModel.java" in gui/openoffice).
separating oostyle
like separating oostyle
I don't feel that I understand your goal well enough.
Perhaps it will become clearer what class belongs where soon (it is not necessarily always clear-cut).
The direct goal is separation of concerns.
oostyle: given a .jstyle, a list of citation groups (in order of
appearance) and a list of citations (citation keys) in each group,
and access to some BibDatases, produce citation
markers for these groups and a bibliography according to the style. It needs to communicate
formatting instructions, uses OOFormattedText for this purpose.
In principle this functionality does not depend on if we use LibreOffice,
OpenOffice, MS Word, or generate a HTML page, as long as we can feed it the necessary data,
and interpret the markup it produces into the target.
It might also produce cross-references between citations and bibliography entries
by including links in the markup.
I expect CSL does something like this (style+citations and cites to markup) as well.
logic/openoffice contains a mixture of:
- (1: backend) how do we store/retrieve locations of
the citation groups (reference marks) and list of citations
belonging to them (reference mark names) and associated data
(citep,citet,nocite) in reference mark names, pageInfo in
user-defined document properties), as well as how do we find the
taxt range containing the bibliography for refresh. - (2: OOFormattedText interpreter) the markup received from oostyle needs to be interpreted
into the locations the backend points to. - (3) methods to be called from GUI. Those that change the document need to communicate to the backend.
So oostyle is a part that might be of interest in other contexts.
Correspondingly its main data structures could probably move to "model/oostyle".
From logic/openoffice it is harder to pick.
DocumentConnection is not really a data structure. It is basically an
XTextDocument with some of its relations and interfaces cached in its
other fields. Just as we do it in the constructor, we could always
get them starting from the XTextDocument. The rest is mainly a
collection of methods, some of them already static, others use the
XTextDocument and/or the cached values, but could be made static by
adding an XTextDocument argument. And it has a LOGGER.
OOUtil is mostly concerned with interpreting OOFormattedText into
an OpenOffice or LibreOffice writer document (insertOOFormattedTextAtCurrentLocation2)
It has a small counterpart within oostyle/OOFormat.java : setLocale, setCharStyle, and paragraph
wrap an OOFormattedText into the corresponding tags. Most of the markup comes
from the layout and other options from the jstyle.
The rest of logic/openoffice seems more or less specific to either the backend or the gui,
with OOFrontend in between. I expect to move more support-the-gui code
from OOBibBase here. RangeSortVisual provides the "order of appearance" of
citation groups needed by oostyle. RangeKeyedMapList might possibly be replaced
by or reimplemented using a MultiMap from elsewhere.
abbreviations in names
e.g., try not to abbreviate names.
It says: "try not to abbreviate names of variables, classes or methods"
- The general principle, as I saw before was closer to: try not to abbreviate global names.
- The hard part is: what is the expected balance between avoiding unnecessary verbosity
that just clutters the code and cryptic (or well-known?) abbreviations
(like awt, fxml, slf4j, cli, gui, i (in StringInt),
CSLType, RisMonth, AuthorLF_FF, JabRef :). Unfortunately "cryptic"
depends on who is seeing it, with what background and which state of mind
(as in looking at ones' own code a year later). - Below try not to abbreviate names
I findcatch (IOException ioe)in an official example. This suggests
that local variables are probably exempt.
IOExceptionitself contains an abbreviation, and is a globally used name.
My filenames (and corresponding classnames) are usually
not abbreviations (except the "OO" prefix already in use before,
Compat (would it be more expressive as Compatibility?)
How does it relate to contractions in
"the most important data structures (BibDatabases, BibEntries, ...)",
getPrefsNodeForCustomizedEntryTypes, ParamLayoutFormatter?
-
Seeing
for (LayoutFormatter anOption : option) {reveals that the nameoption
refers to a collection of things with typeLayoutFormatter.
These are namedanOptionwhich contributes nothing to my understanding. Nor would it
help if we called itaLayoutFormatterortheCurrentLayoutFormatter -
My local variables and argument names often are abbreviated: for example
CitationGroup cg.
ReplacingcgwithcitationGroupis about as useful as
changingIOException ioetoIOException ioException:
adds nothing to what we already know from the verbose typename.
I will probably need more explicit examples of which abbreviations
should I expand. I, of course understand them (for now), but it is hard to guess
which ones could appear confusing or cryptic to others, or even myself at another time.
There was a problem hiding this comment.
I will probably need more explicit examples of which abbreviations
should I expand. I, of course understand them (for now), but it is hard to guess
which ones could appear confusing or cryptic to others, or even myself at another time.
Not all JabRef code is up to date, there are unfortunately still parts of the code where the standard is lower than we'd like it to be. When it comes to AuthorLF_FF and some other artifacts of Authors.java/BracketedExpression, they are a mix of code that should be refactored, but no one has had the time to do so, and names/commands that the user is expected to manually enter within JabRef or in an external text editor, without autocompletion, type safety, etc. They probably would have been named differently if they were created today.
Imho https://github.com/antalk2/jabref/blob/improve-reversibility-rebased-03/src/main/java/org/jabref/logic/openoffice/Backend52.java#L236-L240
CitationGroup cg = new CitationGroup(cgid,
sr,
itcType,
citations,
pageInfo);
and similar would be helpful (to me) if they were expanded.
Generally speaking, I'd make a separation between what I'd consider ok in a draft-PR and what I'd consider ready to merge, and I'd still try to err on the side of expanding names, as opposed to abbreviating them. I can't say that I have never written IOException e but I probably should at least have done IOException exception 😬
|
May I ask about the time frame of working on this? How much time do you have to work on this and until which date you want to be finished completely? Some software engineering comments:
Please get back to us if the text was not understandable. |
I was wondering about that, too. What is done, and what is missingThe issue is: loss of pageInfo upon (Merge,Separate) To solve this, started to distinguish CitationGroup and Citation Using these terms, the problem is, that JabRef5.2 associates pageInfo to CitationGroups This is partly hidden by the fact, that Merge does not clean up, it leaves the pageInfo values The proposed solution is: move pageInfo from CitationGroup to Citation. This raises questions about backward compatiblity
To minimize duplication, now both CitationGroup and Citation have pageInfo fields, We have Backend52, collecting code that behaves differently between versions.
To establish context for Backend53, the rest has to be able to handle the corresponding
SummaryI think ManageCitationsDialog and Backend53 are the main missing
Are you referring to the comments in example_style_file.jstyle or
The "rebase B on A" confuses me. Does it mean "rebase B on (main after
|
|
Another idea for the review process would be that you finish the code (on the current branch). Then extract changes that logically belong to each other (say gui changes, OO interface changes, migration,... ) to new PRs that can be reviewed and merged independently. But maybe we have to accept that these are major changes and review everything as one PR |
Basically, yes, that would help. Though in this example, wouldn't
rebase B on last state of A. Same as the headline "Sync using the git rebase:".
Everyone work on JabRef when they have spare time to do so. If there are 15 000+ line changes that must be reviewed the same person must review all the changes (and have enough spare time to do so). If there are several smaller PRs it doesn't have to be the same person doing the full review -> timelier reviews. Additionally, you are likelier to get early feedback, even when the PR is marked as a draft 😃 |
|
@antalk2 could you make a PR to |
How do I do that? pushed to antalk/jabref improve-reversibility-rebased-03 |
|
Looks right to me. I think we can change it afterward as well if it is wrong, or in the worst-case scenario we close/re-open X) |
I was asking, because I am helping persons moving, renovating my house, ... and thus, I have to schedule my freetime accordingly. - I can try to review and give feedback twice a week. Each two weeks, we have a developer's call. If you want, you can join and we can go into details. Besides that, these calls ensure that we have at least a quick look on your PRs each two weeks. I think, the whole thing will take at least 6 weeks to complete.
May I ask why
I totally forgot which part I meant. - Everything developer-facing should go there. This includes code design decisions, explanation of how classes belong together, ... - I think, it was because of a very long text in a JavaDoc, which did not belong to one class, but to a set of classes. For sure, the documentation should be linked form the JavaDoc. For user-facing documentation chagnes --> e.g., other usage, ... Please submit a WIP-PR to https://github.com/JabRef/user-documentation.
We typically like a single commit when we should start to review a PR. Because we go through the code from top to bottom in the GitHub PR view - and do not review commits separately.
Yeah, just merging While addressing comments on
I have no clue, since I am not into the code. Seems you found a way and we need to first focus on getting #7787 merged. It seems that each of the new PR is "somehow" self-contained. Thus, I ask to explain the self-containtness explicitley at #7787 (comment). This really helps others to give proper feedback. Maybe, I forgotten to state: The feedback we give is increase maintability of the code by other persons than the original author. The nature of the JabRef project is that contriubtors come and go, but the code stays. Thus, we try to establish a proper level code "qualtiy". For sure, everyone has its own style. However, we try to establish a general style in JabRef (mostly following Java by Comparison).
I think, Optional is good as class variables to really state that these can be null. |
This link points to a list of subjects, but I see no "contact
The oobranch series is organized so that after each layer the code can be compiled.
Apart from the general description: UNO utilities, general utilities,
You are probably doing it wrong. You try to read the files in alhapbetic order.
I think it is time to make up your minds, will you read it, scan and decide
Yes, 53 for "some future version". ok.
Is JavaDoc (in the sense using
That book is paywalled. On the available samples: http://media.pragprog.com/titles/javacomp/boolean.pdf
Goes from (v1) to (v2)
Suggests: But why not:
Summary
Probably in any statically typechecked language: it breaks the promise
The presentation says: "[in java] every reference type must include null".
|
|
I believe this pull request is obsolete, replaced by oobranch-[a-j] |
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)Updated to reflect changes in fix-7454
This is not finished.
Please comment if you feel some of the changes go in the wrong direction.