Add preferences window #176
Conversation
|
Amazing, thanks a lot! I'll review this soon. |
|
Overall it looks good. Thanks! Like you say, a way of migrating all your citations from one format to the other would be needed, otherwise Cita will fail to read any notes in the other format. Would you feel comfortable adding that? Otherwise, a good replacement for now might just be a description warning that changing this setting won't migrate notes - so it should only be changed when first setting up Cita. I also commented where you wait for |
|
Thanks for reviewing! I'll try adding the migration between notes and extras. It shouldn't be too difficult as all of the code for parsing and saving citations is already there in sourceItemWrapper.js. Regarding more detailed descriptions, perhaps it would be enough to just have some description text in the group box. Something like: "Cita can save citation data either as a note or in the extra field of library items. As using the extra field for citation data will make it impossible to use it for other purposes, the recommended option is storing citation data in notes. Changing this setting will migrate existing citation data to the new storage location for all items in the library." |
|
From my perspective the use of notes in recommended so I don't think we
should invest a whole lot of time in supporting the workflow of
continually changing between storage types. If switching storage types
also migrates all citations, then it shouldn't (normally) be possible to
get citations stored in two different ways at once?
Unless you meant if the extra field already has some other info in it?
The function for writing to the extra field should be able to read/write
citations without affecting any other data that might be in there.
That's just off the top of my head though - if you encountered some
conflicts / weirdness let me know!
That sounds good for the description!
…On 4/08/2022 11:22 am, Robert Lasch wrote:
Thanks for reviewing!
I'll try adding the migration between notes and extras. It shouldn't
be too difficult as all of the code for parsing and saving citations
is already there in sourceItemWrapper.js.
The question is what we should do if we are migrating from note to
extra and the extra field is already used for some items. Would it be
enough to warn the user once that data could be overwritten or should
we ask for each item that already has data in the extra field?
Regarding more detailed descriptions, perhaps it would be enough to
just have some description text in the group box. Something like:
"Cita can save citation data either as a note or in the extra field of
library items. As using the extra field for citation data will make it
impossible to use it for other purposes, the recommended option is
storing citation data in notes. Changing this setting will migrate
existing citation data to the new storage location for all items in
the library."
Do you think that would work?
—
Reply to this email directly, view it on GitHub
<#176 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGM5S3HE64ZVHHHMHLMDHX3VXODUDANCNFSM55EQYLEA>.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Thanks so much @RoBaaaT for working on this, and thank you @Dominic-DallOsto as usual for taking care of reviewing it.
I added two comments above concerning this.
As Dominic pointed out:
Thus, the following would be incorrect:
As described in #13, when I started working on this project, I decided to use the extra field. But it soon became obvious that Cita was storing too much information there (remember that we store metadata of the cited items), and the extra field became too cluttered. That's why in the end I decided to migrate to using note attachments. |
|
Hi, thanks again for your reviews and sorry for taking a while to respond. I have now made some more changes to
Thanks for mentioning that storing citation data in the extra field does not mean that no other data can be stored in the field. I was not aware of that, and I have changed my proposed description text for the storage preference to reflect this (you can find it in To make it so that the preference dialog would have the same style as the rest of Zotero, I had to change its "wrapper" file ( |
|
Thanks a lot for this!
That sounds like it addresses all the issues. I'll test this within the
next few days but it all sounds good.
…On 17/08/2022 5:41 pm, Robert Lasch wrote:
Hi, thanks again for your reviews and sorry for taking a while to respond.
I have now made some more changes to
* add a short description text to the storage preference
* pass the Prefs object as a window argument instead of using the
weird hack to wait for it to be set from bootstrap.js
* migrate the storage location of existing citation data when the
preference is changed
* give the preference dialog the same style as the rest of the
Zotero app
Thanks for mentioning that storing citation data in the extra field
does not mean that no other data can be stored in the field. I was not
aware of that, and I have changed my proposed description text for the
storage preference to reflect this (you can find it in
|wikicite.properties| at |wikicite.prefs.citation-storage-desc|).
To make it so that the preference dialog would have the same style as
the rest of Zotero, I had to change its "wrapper" file
(|preferences.html|) from .html to .xul. I was not able to get the
proper styling using the HTML version, despite including the Zotero
stylesheets there, and I also could not find other examples of people
using .html for additional windows in Zotero extensions, so changing
to .xul - which everyone is using - seemed like the simplest option.
Please let me know if you have other ideas.
—
Reply to this email directly, view it on GitHub
<#176 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGM5S3FVRGV7A3H3BIDEAWLVZUBZBANCNFSM55EQYLEA>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hey, thanks a lot! This works really well. Just a couple of tiny things I left comments about. Then I think we're good to go! |
|
I added a Also, do the progress bar messages look cut-off for you, or is that just on my end? But then I think (for real) it's all good! |
|
Sorry for taking a while, but with my latest change I've improved the migration process to either migrate all citation data or none at all if any errors occur (in that case the preference also remains unmodified). This is achieved by making all changes in a Zotero DB transaction, which is rolled back in the case of errors. If errors occur, the titles of items that caused the errors are collected and shown to the user in an alert. This way the user can manually check the citation data on these items and try to change the storage preference again at a later point in time. The migration also now properly uses async/await calls, which makes it so that the progress reporting actually works as intended (although you might still only see the "migration finished" message if you have few items in your library). |
Do you mean the progress "window" in the bottom right? I've noticed too that those sometimes look cut off, but for me this is not consistent. Most of the time they look totally fine. Maybe this is an issue with the underlying |
|
Amazing, thanks a lot! That sounds like the perfect approach. I'll
review this soon.
In the meantime I've managed to get testing mostly working (without
codecov but otherwise fine). But I'll merge this first then add tests later.
Thanks a lot!
…On 3/10/2022 5:08 pm, Robert Lasch wrote:
Sorry for taking a while, but with my latest change I've improved the
migration process to either migrate all citation data or none at all
if any errors occur (in that case the preference also remains
unmodified). This is achieved by making all changes in a Zotero DB
transaction, which is rolled back in the case of errors. If errors
occur, the titles of items that caused the errors are collected and
shown to the user in an alert. This way the user can manually check
the citation data on these items and try to change the storage
preference again at a later point in time.
The migration also now properly uses async/await calls, which makes it
so that the progress reporting actually works as intended (although
you might still only see the "migration finished" message if you have
few items in your library).
—
Reply to this email directly, view it on GitHub
<#176 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGM5S3EOQU32JILEZNX3V7DWBLZHXANCNFSM55EQYLEA>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Ok, that makes sense. Like you say yeah, waiting for the window to be
ready in `index.js` is maybe the best idea for now
…On 4/08/2022 11:01 am, Robert Lasch wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In src/dialogs/preferences/PreferenceDialog.jsx
<#176 (comment)>:
> @@ -0,0 +1,51 @@
+import React, { useEffect, useState } from 'react';
+import PropTypes from 'prop-types';
+
+/* global window */
+
+const PreferenceDialog = (props) => {
+ const [storage, setStorage] = useState('');
+
+ useEffect(() => {
+ init();
+ }, []);
+
+ function init() {
+ // note: this is an ugly hack required because window.Wikicite is only set after the window has loaded, and is thus not available when the PreferenceDialog is first rendered
If I am not mistaken, this will not work because the Wikicite object
that is being passed through window.arguments is different from the
one that is being set to window.Wikicite (which happens in
bootstrap.js:117 on window load). The first one is defined in
wikicite.js and only has utility functions like getting localized
strings, while the second one is defined in bootstrap.js and has the
Prefs property to manage preferences. I do not know why these two
different Wikicite objects exist, but I did not want to make any
changes to them that could potentially break other stuff.
Maybe a cleaner solution for the preferences dialog would be to wait
for window.Wikicite to be set within dialogs/preferences/index.js and
only then render PreferencesDialog, passing in window.Wikicite. This
would at least externalize the weird waiting logic from the
PreferencesDialog React component. What do you think?
—
Reply to this email directly, view it on GitHub
<#176 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGM5S3AZVJ7POT2LXIFKJLTVXOBGTANCNFSM55EQYLEA>.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Do you think it looks nicer with bullet points in front of the paper names?
I haven't quite worked out the details, but I'm getting some odd errors like It seems to happen when I deliberately introduce an error to one of the citation notes (eg. get citations from Wikidata for the item with DOI Could the migration to an extra field not be correctly deleting the citation note after an error? |
Yes, that's definitely better, good idea. I think it makes it a lot more readable, especially with many "bad" items.
In my testing I only had some minor issues with the detail display for selected items sometimes not being updated immediately after the migration, but this could always be resolved by just selecting another item or going to another collection. But I think it could be possible that there are some corner cases where the SourceItemWrapper instance that is being used in the UI code still has some stale data after running (or trying to run) the migration, which is then saved again when something is updated from the UI. I could imagine that this would lead to the types of issues you are seeing. I'll investigate this tomorrow and get back to you. By the way, where are you seeing the errors you mention? In the Firefox developer console? I haven't gotten Zotero with debugging support working yet on my end, so I guess I'll have to look into that again... |
|
Yes, in the firefox dev console. The instructions under ## Debugging section in the README here are fairly comprehensive. I'll try to do up a step by step guide some time soon. One important thing to note is to build with debug support, you need to run |
|
Hi, I tried setting up the debugging on my end, but am not able to get the custom-built Zotero instance to start (using Windows, which may or may not be contributing to the issue): When I start the Zotero instance it does not start up properly but just shows a window with an error message saying that there was an unexpected "menuitem" element in some xul file. It also somehow gets completely stuck and I have to kill the process through taskmanager. I initially thought this may just be a bug in the Zotero HEAD version, but checking out the latest release tag and rebuilding still resulted in the same issue. Just to check, which Zotero version are you using for debugging on your end? As a side node, I also had to specify Looking beyond the startup issue, I am actually able to connect the devtools, which is at least something I guess. Here I initially also had some trouble even figuring out how to enable and start remote debugging in the older Firefox version, as the process has apparently changed in the latest Firefox version. Maybe it would be good to document the steps in the README for others (1. enable remote debugging in the devtools settings; 2. restart Firefox; 3. select "connect..." in developer options), as it took me a while to figure this out. I'll continue trying to get this working, it just might take some time. Maybe you have some feedback that can help. |
|
Hmm, that's odd. If it randomly helps, I'm using Also if it helps, I used WSL instead of Cygwin for building Zotero. Basically, following the instructions here in the WSL terminal, but running Then I could run Now connect the debug tools to port 6100, click "Main Process", and you should see something like this.
You should be able to set breakpoints in the code as shown (the blue arrow on line 31) |
|
Using WSL for building did the trick for me. I'm asking because I've found that breakpoints will only work if I select Cita, but I only get console output if i select the main process, which is kind of inconvenient. However the console output also does not show any Zotero debug messages, only warnings and errors, despite specifying Regarding the issue with the migration you brought up, I've done some more investigation and it looks like the transaction is just not properly getting rolled back if a failure occurs, which causes all items besides the failed ones to be migrated anyways. But I do not yet understand why this is happening. Unfortunately the whole Zotero DB API is also quite sparsely documented, so I'm not sure if I'm just doing something wrong in the way I'm running the transaction. |
|
For me selecting the main process also works with breakpoints once I open the debugger tab like in the image above. About the zotero log I'm not sure actually. On Ubuntu it all shows up in the console output when I run zotero from the command line, but I haven't got this to work in Windows. Does the Zotero JS debug console have the messages you're after? Not sure if it helps but here and here the pattern seems to be using I quickly checked the source here but didn't immediately find any answers. You're right, the documentation doesn't have much to go on. |
Ahh, I found the solution for this here. Basically, on windows you need to put |
|
Not sure how but rethrowing the errors we get inside the loop seems to fix this problem for me. It doesn't make sense to me why this is the case but Steps to reproduce for a fresh library:
|
|
Thanks for the proposed fix, I've added a comment on that. Meanwhile I've also been doing some investigation and found that the transaction is actually getting rolled back correctly (at least in the database itself). If you restart Zotero after the failed migration, everything will be as it was before attempting the migration. So there seems to be some inconsistency between Zotero's in-memory representation of the item data and the actual database. Do you know if there is any caching layer or something similar that needs to be considered when running transactions that may roll back? I've been looking at some of the code in |
|
Hmm yeah, you're right. That's interesting. The only mention of a cache I've seen so far is here
|
|
This sounds like the same issue. |
Indeed, that seems to be the same issue. I'm wondering whether there is any way to trigger a full reload of Zotero's state from the database for use cases like ours. Anyways, as a simple fix for our issue I've instead changed the migration to run in two steps (whereas previously it would load the data for an item and then immediately try to migrate it):
If any errors occur during step 1, the migration will abort before even making any changes to the items, which saves us from having to undo changes manually. Of course if errors occur during step 2, we are going to run into the same issue as before, but this should never happen. In my testing this now seems to work as intended. |
|
Thanks! That sounds like a good workaround. |
This makes it so that the style of the preference window is consistent with the rest of the Zotero application. I was not able to figure out how to achieve this with html.
This change makes migrating the storage location for citation data atomic by running the migration for all items in a Zotero DB transaction. If any problems occur during the migration, the transaction aborts and no changes are made to the items. In this case the storage preference also remains un- changed and the user is alerted about the failed migration. The alert includes a list of item titles that caused the migration to fail, so that the user can at least manually try to fix the broken citation data.
Dominic-DallOsto
force-pushed
the
RoBaaaT/issue35
branch
from
c09e52e to
16258a8
Compare
|
Perfect, this works nicely for me now. And also after rebasing. Thanks again for all the great work and sticking through the debugging process! |
This partially addresses #35 by implementing a basic preferences window.
The new preference window allows users to change the storage preference for citations (extra/note). The default was changed to 'note' to match the previous behaviour of the extension.
What is potentially missing is to move existing citations from the 'extra' to the 'note' format and vice-versa when the preference is changed by the user.
This pull request does not add the other potential preferences listed in #35, as the underlying functionality for those preferences has not been implemented yet.
It would probably make most sense to add those preferences to the window when the respective issues (#82, #45, #14) are addressed.