WIP: Improve reversibility rebased 03 #496
Conversation
Is still true? If that is the case, I'd suggest that we bring the code style more in line with the rest of the JabRef code. BackEnd53 and the migration would probably be better in a separate PR. |
They are still missing, yes. Just finished separation of formatting
ok. To see your comments in the code, I pull from antalk/jabref? |
|
add formatBibliography 31b3b38 has inifinite recursion |
|
Comments are not added yet. You’ll be able to see them on this page I just ran out of time today.
…On May 6, 2021 at 17:25:14, antalk2 ***@***.******@***.***)) wrote:
add formatBibliography 31b3b38(31b3b38) has inifinite recursion
Tried to push, that failed. Tried to pull, now shows a lot of conflicts.
Did you mass-format the code? Or did I do something like pulling from the
wrong location?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub(#496 (comment)), or unsubscribe(https://github.com/notifications/unsubscribe-auth/AAT2NZ4FTLIQRP2U6CPV5ELTMMCLVANCNFSM44H2SPMA).
|
You managed to get this worked out? Just a last sanity check for me, the starting point was that it is impossible to "re-merge" split citations in the open office document? E.g., if you had the in-text citations Btw, which code editor is it that you are using? I am just wondering if it is possible to make it display the code style used in most of JabRef's code differently. |
| NoSuchElementException, | ||
| WrappedTargetException, | ||
| UnknownPropertyException, |
There was a problem hiding this comment.
Do you mean the line breaks, or the exceptions?
The line breaks technically no.
Some of these exceptions mentioned here I caught below,
(UnknownPropertyException, WrappedTargetException)
but not all.
The contructor itself is called from ManageCitationsDialogView.initialize()
@FXML
private void initialize() throws NoSuchElementException, WrappedTargetException, UnknownPropertyException, JabRefException {
There is no visible call to initialize() in the class body, presumably accessed through the @FXML magic,
I guess it is called as an event handler or action.
I have no definit answer to "where should these exceptions be caught": should we catch them all
here, or pass them all up (to initialize? or above that?), or something in between. We are probably choosing between
calling a dialog box from an event handler and throwing exception from one.
Those I caught were probably caught here, because this is where they reached
the GUI level. I may have seen UnknownPropertyException and caught it here,
I am not sure I have ever seen a WrappedTargetException. Maybe.
Summary: I do not know where should they be caught.
Moved them to the catch below, then dropped NoSuchElementException,
since the compiler now noticed it is not thrown anyway.
There was a problem hiding this comment.
Do you mean the line breaks, or the exceptions?
Exceptions.
There is no visible call to initialize() in the class body, presumably accessed through the @FXML magic,
I guess it is called as an event handler or action.
I haven't spent that much time with JavaFX, but pretty much
There was a problem hiding this comment.
I have no definit answer to "where should these exceptions be caught": should we catch them all
here, or pass them all up (to initialize? or above that?), or something in between. We are probably choosing between
calling a dialog box from an event handler and throwing exception from one.
I'd say catch them all here, or latest in the View. If appropriate do some action (although in this case, I am not sure if you can do all that much more than display an error dialog to the user). I'll try to update the documentation when I have time.
I'll comment in the code what I think of each and why but quite frankly, some of them I have no idea what to do with either (WrappedTargetException would be one of those).
As a general rule can't think of any reason to make a JabRefException (or any other) bubble up past the View. I'd say that most of them (all?) should be handled latest in the ViewModel. This would be one of those I'd appreciate a second opinion on @Siedlerchr 😛
src/main/java/org/jabref/logic/openoffice/DocumentConnection.java
Outdated
Show resolved
Hide resolved
No, the opposite: in jabref52 you can cite as you like, creating individual citations, (or groups if you select Unless you are ready to print now you have a problem: you cannot remove individual citations Aside: If for simplicity we identify small groups with "citations", the "Merge" could be "Group", and "Separate"
Minimally: you move the pageInfo from CitationGroup to Citation.
There is a question I already mentioned: do we have to support working with documents in the old format, that is context identifying the group and citations below in order, would probably In another line of ideas, instead of coding relations into reference mark names and property names, This would both allow the user to modify its content without specialized dialogs
Do you mean the improved automatic line breaks you have shown me in IDEA? In principle "almost anything" could be done using the builtin LISP and if needed On the line breaks you already know what do I think: dogmatically avoiding linebreaks Now that you mentioned, starting from https://www.emacswiki.org/emacs/LineWrap The projects spacing rules, that insist on spaces around operators "a + b", not "a+b" and do not allow spaces nameOfOThing.nameOfAMethod(widthOfAThing / heightOfAThing).andSomeMore()
That is harder to discern, than one would hope: my first impression from OOBibBase |
| DocumentConnection documentConnection = this.getDocumentConnectionOrThrow(); | ||
|
|
||
| // Access the text document's multi service factory: | ||
| mxDocFactory = UnoRuntime.queryInterface(XMultiServiceFactory.class, mxDoc); | ||
| checkIfOpenOfficeIsRecordingChanges(documentConnection); |
There was a problem hiding this comment.
I'd suggest refactoring these. I haven't looked at all usages of getDocumentConnectionOrThrow and checkIfOpenOfficeIsRecordingChanges but I am not sure that you want to throw an exception here (see https://github.com/tatsuya/effective-java/blob/master/chapter-9.md#chapter-9-exceptions).
Both checks are before you attempt to fetch the reference marks and might fall under the https://github.com/tatsuya/effective-java/blob/master/chapter-9.md#item-59-avoid-unnecessary-use-of-checked-exceptions category.
There was a problem hiding this comment.
These (and some others) are testing preconditions for the GUI action they are called in.
I need to communicate to the GUI, or consider OOBibBase itself part of the GUI and
bring up a dialog. Most of OOBibBase did not do such things (except the select which document to connect part).
So the optiions I see are:
-
Move the precondition tests to the caller in (most likely) OOPanel.
Question: should the caller know about such details of the action?
Some of them could be truned into a warning: for example
missing character and paragraph styles in the document:
we could still run, just skip applying those missing.
Or install a version and inform the user we did and that he can
edit them. -
Bring up dialog within the action.
Question: do error dialogs belong in the action implementation? -
extend the return types of the actions to include diagnostics.
Something like this is already there in exportCitedHelper,
where the list of unresolvedKeys and the result database are packed together.
For the "cannot do it" cases this does not seem practical.
There was a problem hiding this comment.
- It depends on the exception and the caller. At the moment I am looking at
ManageCitationsDialogViewModeland I believe it is appropriate that it is aware of the failure to fetch citation entries (e.g., use an optionalpublic Optional<List<CitationEntry>> getCitationEntries(), perhaps close the manage citation dialog if the optional is empty?). I didn't notice that OOBibBase was part of the GUI (gui/openoffice), but since it is I am assuming it is intended as a ViewModel and I'd consider it appropriate that it is the one that shows an error dialog. - What is "the action"? The lambda functions associated with a button? If that is what you referred to, I'd say preliminarily yes. If there are larger chunks of code you can break them out in their own method as well, similar to
OpenOfficePanel#connectAutomatically. - There are some cases where this is appropriate. As a personal preference, I try to avoid it, but it isn't always possible without making the code unreadable.
There was a problem hiding this comment.
- It depends on the exception and the caller.
At the moment I am looking atManageCitationsDialogViewModel
and I believe it is appropriate that it is aware of the failure to fetch
citation entries (e.g., use an optional
public Optional<List<CitationEntry>> getCitationEntries(),
perhaps close the manage citation dialog if the optional is empty?).
Some of the checks are already in OpenOfficePanel, called before creating ManageCitationsDialogView, in
manageCitations.setOnAction
A problem (I think) with the code there
is that the constructor tries to get the data, but in some cases it cannot.
A constructor can only throw or return an instance encoding "I am not usable".
Technically it is not the constructor of ManageCitationsDialogView, it is its initialize() method,
which passes ooBase to new ManageCitationsDialogViewModel(ooBase, dialogService);
which calls ooBase.getCitationEntries();
So deep down in dialog construction we start to ask for details
we sometimes cannot get.
I would suggest that we should not try to build and show a dialog we cannot fill.
I do not know how to close a dialog we are currently building and is (not itself, its parent) going to be
the argument (or was, depending on when initialize is called)
of showCustomDialogAndWait when we return from the constructor to initialize() and from there ... I do not know,
the caller of initialize().
The tests themselves may be duplicated to have a throwing and a returning version.
Ideas
(1): move the call ooBase.getCitationEntries(); to before constructing
ManageCitationsDialogView. We can then decide based on its return value
what todo. If it succeeds, we can pass the data to the dialog constructor (we still need to pass ooBase,
as long as the dialog wants to call ooBase.applyCitationEntries)
(2) add ooBase.getCitationEntriesPretest(); to be called before the constructor.
But in this case ooBase.getCitationEntries(); might still fail for reasons we did not or could not
test without actually tyring to collect the data.
I didn't notice that OOBibBase was part of the GUI (
gui/openoffice),
but since it is I am assuming it is intended as a ViewModel
and I'd consider it appropriate that it is the one that shows an error dialog.
I think it probaly predates the gui/logic separation, or as a plugin it was not considered
subject to it. A lot of its content does not belong to gui, but was not easy to separate, so it stayed
in gui due to its use of dialog.
- What is "the action"? The lambda functions associated with a button?
If that is what you referred to, I'd say preliminarily yes.
If there are larger chunks of code you can break them out in their own method as well,
similar toOpenOfficePanel#connectAutomatically.
You pointed to a method in OOBibBase, which is technically in gui, but mostly consists
of methods that should be in logic. In my head there is a separation between "real gui"
that is about opening dialogs, and methods that "implement the actions" (I referred to these).
These need to be careful and test what they have (connection, necessary conditions to work)
in order to avoid messing up the document by failing during modifications. It feels safer to include the checks inside,
otherwise we rely on the gui calling the necessary checks (while the list of those may still evolve).
But what I thought of as "action implementation" can still be split to "actual implementation" and a wrapper
that does the checks and shows dialogs. Maybe OpenOfficePanel could export
showNoDocumentErrorMessage() and other methods (or they could be moved out)
and delegate calling them to other modules.
It is probably not the right time to start reorganizing even more files.
What do you think about the (1): move the call ooBase.getCitationEntries(); to before constructing
solution?
extend the return types of the actions to include diagnostics.
- There are some cases where this is appropriate.
As a personal preference, I try to avoid it, but it isn't always possible without making the code unreadable.
In this respect java is strange: there is Optional with map, but this latter
will not allow checked exceptions. It does not have Pair (except where they needed it) and it does not have
Either<T,U> or Result<Value,Messages> which could allow such extension wihout
creating a new class for each case.
There was a problem hiding this comment.
(Part of) The reason why I am harping about returning an Optional, is that using JavaFX we can bind to an optional value in the ViewModel and display an error message and call this.close() if it ever becomes Optional.empty() while the Window/Dialog is shown. Say something happens while you have the window/dialog open (e.g., in a background thread or if you manually refresh the list), and we know you can no longer access the document. I am not too fond of JavaFX but it is awfully convenient sometimes.
In my head there is a separation between "real gui"
that is about opening dialogs, and methods that "implement the actions" (I referred to these).
You are referring to View vs ViewModel, where View is the "real GUI"?
I do not know how to close a dialog we are currently building and is (not itself, its parent) going to be
the argument (or was, depending on when initialize is called)
of showCustomDialogAndWait when we return from the constructor to initialize() and from there ... I do not know,
the caller of initialize().
Can't we call this.close() in the constructor? ManageCitationDialogView has access to viewModel.citationsProperty() (which can be made Optional).
What do you think about the (1): move the call ooBase.getCitationEntries(); to before constructing
solution?
I think I'll have to get back to you on that one. Preliminarily I'd consider OpenOfficePanel to be part of the View even if it seems to contain more logic than it should (which is why I am hesitating about adding more checks there).
You are right in that perhaps now isn't the time for more refactoring. I'll have to think about it it, sometimes small refactoring of old code leads to a lot fewer total changes. I'll start digging into in OpenOfficePanel (instead of ManageCitationDialogView) and get back to you.
There was a problem hiding this comment.
Maybe OpenOfficePanel could export
showNoDocumentErrorMessage() and other methods (or they could be moved out)
and delegate calling them to other modules.
I'd guess the "or they could be moved out" will be the easiest way of avoiding duplicating code and keeping the "information" (e.g. citations or status of the document) up to date.
There was a problem hiding this comment.
(Part of) The reason why I am harping about returning an Optional, is that using JavaFX we can bind to an optional value in the ViewModel
You mean an invisible "property" and an addListener?
and display an error message and call
this.close()if it ever becomesOptional.empty()while the Window/Dialog is shown.
Does this work in initialize? Or only after we returned from it (or from the constructor?)?
Say something happens while you have the window/dialog open (e.g., in a background thread
or if you manually refresh the list), and we know you can no longer access the document.
I am not too fond of JavaFX but it is awfully convenient sometimes.
To manually refresh the list only seems meaningful if we pushed some changes to the document.
We do not do that now, the only action that goes back to the document also closes the window.
In my head there is a separation between "real gui"
that is about opening dialogs, and methods that "implement the actions" (I referred to these).You are referring to View vs ViewModel, where View is the "real GUI"?
No, I think of the ViewModel as part of the "real gui". We leave that when it calls
to methods that work on the document and are not using dialogs or GUI properties.
I this context the separation seems to be:
GUI provides the style, preference values, list of databases and reference to ooBase,
and calls methods that only use these and the document.
I do not know how to close a dialog we are currently building and is (not itself, its parent) going to be
the argument (or was, depending on when initialize is called)
of showCustomDialogAndWait when we return from the constructor to initialize() and from there ... I do not know,
the caller of initialize().Can't we call this.close() in the constructor?
Is this a question, or a statement in question form?
I do not know: if at the moment we call close() the window
is not yet set up properly, the result might be unexpected.
But if close() just emits an event, then it might be in queue
until we return and the window is set up. Closing a window
within its constructor is not necessarily what the library authors expected
to happen.
ManageCitationDialogView has access to
viewModel.citationsProperty()(which can be made Optional).
I guess then initialize could add the listener after it constructed the viewModel.
That would cover the later updates. If we got Optional.empty() within the viewModel
constructor, the listener will probably not called. With a direct check we are back to
calling close from initialze()
What do you think about the (1): move the call ooBase.getCitationEntries(); to before constructing
solution?I think I'll have to get back to you on that one.
Preliminarily I'd consider OpenOfficePanel to be part of the View
even if it seems to contain more logic than it should (which is why I am hesitating about adding more checks there).
We have different views of the dialog. Your image seems to be that things happen while it is open.
Probably apropriate for the case when multiple things can be done without closing the dialog.
My view is more of a temporary dialog: we send a list in and get an updated list back.
The dialog could return this list when closed and OpenOfficePanel could call OOBibBase with the result.
This, of course could not work if the dialog allowed several different actions modifying and rereading the
document. Is this the direction we expect this dialog to move?
You are right in that perhaps now isn't the time for more refactoring. I'll have to think about it it, sometimes small refactoring of old code leads to a lot fewer total changes. I'll start digging into in OpenOfficePanel (instead of ManageCitationDialogView) and get back to you.
I started to move catching the exceptions (and partly not throwing them) to OOBiBBase.
The caller becomes cleaner this way (it has nothing to catch), while the part in OOBibBase becomes more complicated.
It seems meaningful to move handling closer to those throwing.
There was a problem hiding this comment.
Experiment with ManageCitationsDialogView.close()
Can't we call this.close() in the constructor?
We can call it from initialize()
Of the attempts here only the last (variant 4, using Platform.runLater())
succeeded in closing the window, although it did not prevent it from appearing for a fraction of a second.
The others did not have an observable effect.
(The ManageCitationsDialogView thisDialog = this; line is only needed once.)
I'd say that we don't need to support them, just make sure to warn and get a confirmation from the user before migrating.
There is more to it than that. Someone has to do the code modernization and it is hard to find time for it (or volunteers). I have a personal vendetta against |
In Backend52:
- use a single expression
- expand comment
- rename refMarkName to markName
Collectors.joining(",") moved to Codec52.getUniqueMarkName
No description provided.