Porting to 5.0

[Emiliano Heyns]

How do I get a working 5.0? Is there a nightly, or a build procedure? I'd like to take a stab at porting BBT to 5.0, so I want to set up my test env.

[Dan Stillman]

It's the 'master' branch on GitHub:

https://www.zotero.org/support/dev/source_code#working_with_git_repositories

There's a vastly expanded test suite that you can run to make sure
things are working locally.

Note that the translation and style architectures are both still
incomplete, which might impair some of your BBT work. Web translation
works in some cases, but import/export translation might not. Probably
plenty else to work on, though. Things are pretty different, so let me
know if you have any questions.

To be safe, this branch shouldn't be synced with a production server
account yet.

[Emiliano Heyns]

On Monday, February 22, 2016 at 9:20:45 PM UTC+1, Dan Stillman wrote:

It's the 'master' branch on GitHub:

https://www.zotero.org/support/dev/source_code#working_with_git_repositories

There's a vastly expanded test suite that you can run to make sure
things are working locally.

Super. How do I setup the tests? 

 

Note that the translation and style architectures are both still
incomplete, which might impair some of your BBT work. Web translation
works in some cases, but import/export translation might not. Probably
plenty else to work on, though. Things are pretty different, so let me
know if you have any questions.

I'm not too worried about import/export working right now. Since the translators are sandboxed, I figured only minimal changes would be needed once that's done. My main worry is promisification -- as you probably know, BBT does a fair-bit of monkey-patching to do its thing (including a controlled puncture of the translator sandbox BTW), and promise-chains are as a side effect super effective against monkey patching. I need to start looking what parts of BBT are going to survive the transition.

 

To be safe, this branch shouldn't be synced with a production server
account yet.

Fo sho. I don't sync any of my dev profiles.

[Dan Stillman]

On 2/22/16 3:52 PM, Emiliano Heyns wrote:

On Monday, February 22, 2016 at 9:20:45 PM UTC+1, Dan Stillman wrote:

There's a vastly expanded test suite that you can run to make sure
things are working locally.

Super. How do I setup the tests?

./test/runtests.sh

(Pass -h to see available flags.)

[Emiliano Heyns]

When running on latest master, I get:

$ ./test/runtests.sh

Mozilla Firefox 46.0.1

Possibly unhandled rejection:

Must have a source and a callback

[Exception... "Must have a source and a callback"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://zotero/content/xpcom/file.js :: Zotero.File</this.getContentsAsync :: line 225"  data: no]

    Zotero.File</this.getContentsAsync@chrome://zotero/content/xpcom/file.js:225:4

    Zotero.Schema</_updateBundledFilesAtLocation<@chrome://zotero/content/xpcom/schema.js:595:19

From previous event:

    Zotero.Schema</this.updateBundledFiles<@chrome://zotero/content/xpcom/schema.js:477:21

From previous event:

    Zotero.Schema</this.updateSchema</</<@chrome://zotero/content/xpcom/schema.js:94:13

From previous event:

    Zotero.Schema</this.updateSchema</<@chrome://zotero/content/xpcom/schema.js:91:5

From previous event:

    Zotero.Schema</this.updateSchema<@chrome://zotero/content/xpcom/schema.js:89:11

From previous event:

    _initFull<@chrome://zotero/content/xpcom/zotero.js:578:25

From previous event:

    init/<@chrome://zotero/content/xpcom/zotero.js:436:12

    self.setTimeout/<.notify@resource://zotero/bluebird.js:54:5

From previous event:

    init@chrome://zotero/content/xpcom/zotero.js:195:10

    ZoteroService@file:///Users/emile/zotero/zotero/components/zotero-service.js:340:4

    XPCOMUtils__getFactory/factory.createInstance@resource://gre/modules/XPCOMUtils.jsm:326:19

    @chrome://zotero/content/include.js:1:14

[Dan Stillman]

On 6/5/16 7:55 AM, Emiliano Heyns wrote:

When running on latest master, I get:

$ ./test/runtests.sh

Mozilla Firefox 46.0.1

Possibly unhandled rejection:

Must have a source and a callback

[Exception... "Must have a source and a callback"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://zotero/content/xpcom/file.js :: Zotero.File</this.getContentsAsync :: line 225"  data: no]

You didn't do a recursive checkout.

Run 'git submodule update --init --recursive' to fix.

[Emiliano Heyns]

OK, that get's me a little further:

Mozilla Firefox 46.0.1

Possibly unhandled rejection:

xmlhttp is undefined

TypeError: xmlhttp is undefined

    Zotero.Schema</this.updateFromRepository<@chrome://zotero/content/xpcom/schema.js:1062:7

From previous event:

    Zotero.Schema</this.updateBundledFiles<@chrome://zotero/content/xpcom/schema.js:489:11

[Dan Stillman]

On 6/5/16 6:36 PM, Emiliano Heyns wrote:

OK, that get's me a little further:

Mozilla Firefox 46.0.1

Possibly unhandled rejection:

xmlhttp is undefined

TypeError: xmlhttp is undefined

    Zotero.Schema</this.updateFromRepository<@chrome://zotero/content/xpcom/schema.js:1062:7

If you're getting that then a translator update request at startup is failing for you. I've fixed the handling of that, which is what you were running into, but you'll have to see what the actual error is now.

[Emiliano Heyns]

That gets me further still. I still get errors in the sync part of the test (but perhaps that's to be expected), and also:

  Zotero.File

    #getContentsAsync()

    ✓ should handle an empty file (486 ms)

    ✓ should handle an extended character (6 ms)

    ✓ should handle an extended Windows-1252 character (10 ms)

    ✓ should handle a GBK character (5 ms)

    ✓ should handle an invalid character (5 ms)

    #copyDirectory()

    ✓ should copy all files within a directory (52 ms)

    #zipDirectory()

    ✓ should compress a directory recursively (83 ms)

  Zotero.Fulltext

console.error: CustomizableUI: 

  unable to normalize widget

    #indexItems()

    ✖ [FAIL] "before all" hook

      Error: Download failed with status 2153390067 at

        Error: Download failed with status 2153390067

            Zotero.File</this.download</<@chrome://zotero/content/xpcom/file.js:405:21

            NetUtil_asyncFetch/<.onStopRequest@resource://gre/modules/NetUtil.jsm:128:17

        

    #downloadPDFTool()

    ✖ [FAIL] should install the PDF tools

      Error: Download failed with status 2153390067 at

        Error: Download failed with status 2153390067

            Zotero.File</this.download</<@chrome://zotero/content/xpcom/file.js:405:21

            NetUtil_asyncFetch/<.onStopRequest@resource://gre/modules/NetUtil.jsm:128:17

        

    #getUnsyncedContent()

    ✖ [FAIL] "before all" hook

      HTTP GET https://www.zotero.org/download/xpdf/latest.json failed with status code 0 at

        HTTP GET https://www.zotero.org/download/xpdf/latest.json failed with status code 0

            (No stack trace)

I'm still wholly freaked out by the promise-and-yield stuff. Is there any tutorial that could ease me into this? It's a little much to take in all at once.

[Emiliano Heyns]

The test run ends with:

739/745 tests passed

Possibly unhandled rejection:

this._potentialTranslators is undefined

TypeError: this._potentialTranslators is undefined

    Zotero.Translate.Base.prototype.complete@chrome://zotero/content/xpcom/translation/translate.js:1397:1

    Zotero.Translate.Web.prototype.complete@chrome://zotero/content/xpcom/translation/translate.js:2025:20

    Zotero.Translate.Base.prototype.getTranslators/<@chrome://zotero/content/xpcom/translation/translate.js:1171:4

    self.setTimeout/<.notify@resource://zotero/bluebird.js:54:5

From previous event:

    Zotero.Translate.Base.prototype.getTranslators@chrome://zotero/content/xpcom/translation/translate.js:1115:10

    Zotero_Browser.Tab.prototype.detectTranslators<@chrome://zotero/content/browser.js:825:3

From previous event:

    Zotero_Browser</contentLoad@chrome://zotero/content/browser.js:358:3

[Dan Stillman]

On 6/6/16 4:25 AM, Emiliano Heyns wrote:

That gets me further still. I still get errors in the sync part of the test (but perhaps that's to be expected)

No, all tests should work (barring intermittent timeouts from hard-to-debug race conditions).

    Zotero.Fulltext

console.error: CustomizableUI: 

  unable to normalize widget

    #indexItems()

    ✖ [FAIL] "before all" hook

      Error: Download failed with status 2153390067 at

        Error: Download failed with status 2153390067

            Zotero.File</this.download</<@chrome://zotero/content/xpcom/file.js:405:21

            NetUtil_asyncFetch/<.onStopRequest@resource://gre/modules/NetUtil.jsm:128:17

2153390067 is SEC_ERROR_UNKNOWN_ISSUER [1], so HTTPS requests to zotero.org are getting intercepted. You'll have to fix your connection. (And I'll try to get it to log a better error for that.)

I'm still wholly freaked out by the promise-and-yield stuff. Is there any tutorial that could ease me into this? It's a little much to take in all at once.

I'm sure there are tutorials, but I'd start by re-reading Aurimas's explanation here [2] and my replies in that thread.


[1] https://james-ross.co.uk/mozilla/misc/nserror?0x805A1FF3
[2] https://groups.google.com/d/msg/zotero-dev/30l_4rNyXC0/sXWFiPONaVgJ

[Emiliano Heyns]

No idea why my traffic would be intercepted at work -- as I'm connected through a non-managed laptop, they can't be doing it on my laptop, so it'd need to be a transparent proxy of sorts... but I work in the IT dept, and no one around me knows how this could be the case. I have the network boys on it, but they said that even if it were to be the case, I should also have seen warnings from Chrome in usual browsing.

In any case, from a different network, I get 733/741 tests passing. Full log is at https://gist.github.com/be1695f753bcee6babc828fccedb2d49

--
You received this message because you are subscribed to a topic in the Google Groups "zotero-dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/zotero-dev/naAxXIbpDhU/unsubscribe.
To unsubscribe from this group and all its topics, send an email to zotero-dev+...@googlegroups.com.
To post to this group, send email to zoter...@googlegroups.com.
Visit this group at https://groups.google.com/group/zotero-dev.
For more options, visit https://groups.google.com/d/optout.

[Dan Stillman]

On 6/7/16 5:58 AM, Emiliano Heyns wrote:

No idea why my traffic would be intercepted at work -- as I'm connected through a non-managed laptop, they can't be doing it on my laptop, so it'd need to be a transparent proxy of sorts... but I work in the IT dept, and no one around me knows how this could be the case. I have the network boys on it, but they said that even if it were to be the case, I should also have seen warnings from Chrome in usual browsing.

It could be security software as well. The test creates a new Firefox profile, so if something has installed a custom root certificate in your current browsers, this would fail but those wouldn't.

You can test this by adding:

  this.timeout(1000000);
  yield Zotero.Promise.delay(1000000);

inside the first Zotero.Attachments test. That will leave a browser window open, and you can try browsing to https://www.zotero.org and seeing if you get a certificate error. If so you can inspect the certificate.

You can also test from your main browsers: https://www.zotero.org/support/kb/site_certificate_info . The certificate for www.zotero.org should be validated by GeoTrust.

You received this message because you are subscribed to the Google Groups "zotero-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to zotero-dev+...@googlegroups.com.

[Emiliano Heyns]

On Tuesday, June 7, 2016 at 6:47:53 PM UTC+2, Dan Stillman wrote:

It could be security software as well. The test creates a new Firefox profile, so if something has installed a custom root certificate in your current browsers, this would fail but those wouldn't.

It was indeed Avast, who without so much as telling me defaulted to intercepting &*^!*&@^*&# https traffic. Are these people insane?

In any case, with that out of the way:

1. Can I add my own tests, and can I select which tests to run (I'd rather not run the whole Zotero test suite for my own unit tests), and
2. I'm looking at Zotero.Utilities.Internal.itemToExportFormat, but I don't see where the async happens there. itemToExportFormat uses item.getField, which must be async (right?) but it looks like it's called directly rather than using a yield. What am I missing here?

 

[Dan Stillman]

On 6/13/16 3:39 AM, Emiliano Heyns wrote:

1. Can I add my own tests, and can I select which tests to run (I'd rather not run the whole Zotero test suite for my own unit tests), and

Well, you can add files to test/tests/ locally. If you add fooTest.js, you can then run tests in that file with `./test/runtests.sh foo` (or multiple separated by commas), or `./test/runtests.sh -g 'test title to search for' foo` to run specific tests.

If you're not testing something translator-related, it's much quicker to pass -b to skip bundled file installation. -d 5 will display debug output. See ./test/runtests.sh -h for more.

I'm looking at Zotero.Utilities.Internal.itemToExportFormat, but I don't see where the async happens there. itemToExportFormat uses item.getField, which must be async (right?) but it looks like it's called directly rather than using a yield. What am I missing here?

It's not async — the `return {Promise<Object>}` comment is just outdated. `item.getField()` isn't async — item data is loaded either for a whole library at a time (automatically when a library is viewed for the first time in the UI) or when Zotero.Items.getAsync() is used (for rare cases where an item might be accessed before that happens). Either way, once there's a Zotero.Item, all item data is available.

(It's easy enough to confirm this — if the return value doesn't have a .then, it's not a promise.)

[Emiliano Heyns]

On Mon, Jun 13, 2016 at 10:02 AM, Dan Stillman <dsti...@zotero.org> wrote:

On 6/13/16 3:39 AM, Emiliano Heyns wrote:

1. Can I add my own tests, and can I select which tests to run (I'd rather not run the whole Zotero test suite for my own unit tests), and

Well, you can add files to test/tests/ locally. If you add fooTest.js, you can then run tests in that file with `./test/runtests.sh foo` (or multiple separated by commas), or `./test/runtests.sh -g 'test title to search for' foo` to run specific tests.

But this would require me running the tests from a Zotero checkout, right?

I'll just keep using the test harness I already have, I think.

I'm looking at Zotero.Utilities.Internal.itemToExportFormat, but I don't see where the async happens there. itemToExportFormat uses item.getField, which must be async (right?) but it looks like it's called directly rather than using a yield. What am I missing here?

It's not async — the `return {Promise<Object>}` comment is just outdated. `item.getField()` isn't async — item data is loaded either for a whole library at a time (automatically when a library is viewed for the first time in the UI) or when Zotero.Items.getAsync() is used (for rare cases where an item might be accessed before that happens). Either way, once there's a Zotero.Item, all item data is available.

This is different from the 4.0 behavior, right? IIRC, 4.0 would load an item in stages, depending on what fields were requested through getField. Does this mean that if I overlay zoteroPane.xul and init from there, I can be sure the whole library is already in memory? That would mean I could probably ditch the serialization cache part of BBT.

Any particular reason to keep the getField for access? If the item is already in memory, wouldn't something like item.extra be preferable to item.getField('extra'), avoiding the function call overhead?

[Dan Stillman]

On 6/13/16 4:22 AM, Emiliano Heyns wrote:

On Mon, Jun 13, 2016 at 10:02 AM, Dan Stillman <dsti...@zotero.org> wrote:

On 6/13/16 3:39 AM, Emiliano Heyns wrote:

1. Can I add my own tests, and can I select which tests to run (I'd rather not run the whole Zotero test suite for my own unit tests), and

Well, you can add files to test/tests/ locally. If you add fooTest.js, you can then run tests in that file with `./test/runtests.sh foo` (or multiple separated by commas), or `./test/runtests.sh -g 'test title to search for' foo` to run specific tests.

But this would require me running the tests from a Zotero checkout, right?

I mean, sure. Not sure what else you're asking, though, since this is about the Zotero test suite.

I'm looking at Zotero.Utilities.Internal.itemToExportFormat, but I don't see where the async happens there. itemToExportFormat uses item.getField, which must be async (right?) but it looks like it's called directly rather than using a yield. What am I missing here?

It's not async — the `return {Promise<Object>}` comment is just outdated. `item.getField()` isn't async — item data is loaded either for a whole library at a time (automatically when a library is viewed for the first time in the UI) or when Zotero.Items.getAsync() is used (for rare cases where an item might be accessed before that happens). Either way, once there's a Zotero.Item, all item data is available.

This is different from the 4.0 behavior, right? IIRC, 4.0 would load an item in stages, depending on what fields were requested through getField.

Yes. Details explained in Promisification [1] and Deasyncification [2].

But I misspoke slightly. When the library has been loaded in the UI, items can be retrieved with Zotero.Items.get() and will have all data loaded, as explained in Deasyncification.

If the library hasn't been loaded, either the load will need to be triggered [3] or the item will need to be retrieved with `Zotero.Items.getAsync()` and then the relevant data types will need to be loaded on demand (e.g., item.loadItemData()), as was the original plan in Promsification.

Does this mean that if I overlay zoteroPane.xul and init from there, I can be sure the whole library is already in memory?

For UI actions that apply to the currently viewed library, you mean? Yes.

Any particular reason to keep the getField for access? If the item is already in memory, wouldn't something like item.extra be preferable to item.getField('extra'), avoiding the function call overhead?

Primary fields (e.g., .dateModified) are available as properties, but getField() does data transformations on some fields and can return fields in a few different ways. Those could perhaps be avoided if a bunch of other code was reworked, but it also checks for unloaded data, which can happen for libraries that haven't had item data loaded, so removing it would be a little dangerous. We could investigate further, but I suspect the overhead here is worth it.


[1] https://github.com/zotero/zotero/issues/518
[2] https://github.com/zotero/zotero/commit/daf4a8fe4db46ed29de2babed79610a2522821df
[3] https://github.com/zotero/zotero/blob/a200f6cfc52521b1f04427ca33af03b72807654b/chrome/content/zotero/zoteroPane.js#L1234

[Emiliano Heyns]

On Mon, Jun 13, 2016 at 10:53 AM, Dan Stillman <dsti...@zotero.org> wrote:

On 6/13/16 4:22 AM, Emiliano Heyns wrote:

But this would require me running the tests from a Zotero checkout, right?

I mean, sure. Not sure what else you're asking, though, since this is about the Zotero test suite.

Should have explained that. I'm interested in the Zotero test harness, rather than the Zotero tests. I use Selenium for my tests currently, and was looking whether I should move the the harness built into Zotero.

 

I'm looking at Zotero.Utilities.Internal.itemToExportFormat, but I don't see where the async happens there. itemToExportFormat uses item.getField, which must be async (right?) but it looks like it's called directly rather than using a yield. What am I missing here?

It's not async — the `return {Promise<Object>}` comment is just outdated. `item.getField()` isn't async — item data is loaded either for a whole library at a time (automatically when a library is viewed for the first time in the UI) or when Zotero.Items.getAsync() is used (for rare cases where an item might be accessed before that happens). Either way, once there's a Zotero.Item, all item data is available.

This is different from the 4.0 behavior, right? IIRC, 4.0 would load an item in stages, depending on what fields were requested through getField.

Yes. Details explained in Promisification [1] and Deasyncification [2].

Right. OK.

 

But I misspoke slightly. When the library has been loaded in the UI, items can be retrieved with Zotero.Items.get() and will have all data loaded, as explained in Deasyncification.

If the library hasn't been loaded, either the load will need to be triggered [3] or the item will need to be retrieved with `Zotero.Items.getAsync()` and then the relevant data types will need to be loaded on demand (e.g., item.loadItemData()), as was the original plan in Promsification.

BBT really only reacts to 3 things:

1. A right-click on an item
2. A change on an item or the addition of an item to a collection
3. Exports

From the looks of it, any item involved in any of these will have been loaded. Correct? That would simplify a few matters.

[Dan Stillman]

On 6/13/16 5:04 AM, Emiliano Heyns wrote:

BBT really only reacts to 3 things:

1. A right-click on an item
2. A change on an item or the addition of an item to a collection
3. Exports

From the looks of it, any item involved in any of these will have been loaded. Correct? That would simplify a few matters.

Yep. So for those cases, item data access should basically work the same as before.

[Emiliano Heyns]

I'm trying to make database calls, but my code doesn't show any activity. What am I doing wrong here:

Zotero.Promise.coroutine(function*() {

  // this is never reached.

  yield store.queryAsync('CREATE TABLE IF NOT EXISTS lokijs (name PRIMARY KEY, data)');

  return (yield store.queryAsync('INSERT OR REPLACE INTO lokijs (name, data) VALUES (?, ?)', [name, serialized]));

}).then(function() {

  return callback();

}, function(reason) { 

  return callback(new Error(reason));

})["catch"](function(reason) {

  return callback(new Error(reason));

});

[Dan Stillman]

coroutine() just wraps a function. It doesn't run it. So if you want code to run immediately, you need to do Zotero.Promise.coroutine(function* () { ... })(). But generally you just wrap a function in Zotero.Promise.coroutine() and call it from elsewhere.

It's also a bit of an anti-pattern to use callbacks in combination with promises (though there are a few exception). If you need a callback to run after those DB queries, you can use a regular (not-promise-based) try/catch/finally block within the generator. In general, all that callback/error handling code looks overly complicated and unnecessary.

[Emiliano Heyns]

Right, that wasn't particularly smart. I now have this piece of code, which is called from a shutdown listener called from Zotero.addShutdownListener:

Zotero.Promise.coroutine(function*() {

        var err;

        try {

          (yield store.queryAsync('CREATE TABLE IF NOT EXISTS lokijs (name PRIMARY KEY, data)'));

          (yield store.queryAsync('INSERT OR REPLACE INTO lokijs (name, data) VALUES (?, ?)', [name, serialized]));

          callback();

        } catch (err) {

          callback(err);

        }

      })();

but that gets me

FATAL ERROR: AsyncShutdown timeout in profile-before-change Conditions: [{"name":"Sqlite.jsm shutdown blocker","state":{"description":"Waiting for connections to close","state":[{"name":"betterbibtex-lokijs.sqlite#0: waiting for shutdown","state":{"identifier":"betterbibtex-lokijs.sqlite#0","isCloseRequested":false,"hasDbConn":true,"hasInProgressTransaction":false,"pendingStatements":0,"statementCounter":8},"filename":"resource://gre/modules/Sqlite.jsm","lineNumber":259,"stack":["resource://gre/modules/Sqlite.jsm:ConnectionData:259","resource://gre/modules/Sqlite.jsm:OpenedConnection:1122","resource://gre/modules/Sqlite.jsm:openConnection/</<:933"]}]},"filename":"resource://gre/modules/Sqlite.jsm","lineNumber":157,"stack":["resource://gre/modules/Sqlite.jsm:null:157","resource://gre/modules/XPCOMUtils.jsm:XPCU_defineLazyGetter/<.get:198","resource://gre/modules/Sqlite.jsm:ConnectionData:259","resource://gre/modules/Sqlite.jsm:OpenedConnection:1122","resource://gre/modules/Sqlite.jsm:openConnection/</<:933"]}] At least one completion condition failed to complete within a reasonable amount of time. Causing a crash to ensure that we do not leave the user with an unresponsive process draining resources. 

followed by the Mozilla crash reporter popping up.

Should I be doing something to "store" (which is a Zotero.DBConnection I opened earlier) to signify I'm done with it, either after calling the queries to wrap up the query, or at the end of the shutdown listener?

[Emiliano Heyns]

To which I should have added: the data seems to be saved correctly, this error occurs when I close firefox. I have a full console log should that help.

[Dan Stillman]

On 7/3/16 4:53 PM, Emiliano Heyns wrote:

I now have this piece of code, which is called from a shutdown listener called from Zotero.addShutdownListener:

Zotero.Promise.coroutine(function*() {

        var err;

        try {

          (yield store.queryAsync('CREATE TABLE IF NOT EXISTS lokijs (name PRIMARY KEY, data)'));

          (yield store.queryAsync('INSERT OR REPLACE INTO lokijs (name, data) VALUES (?, ?)', [name, serialized]));

          callback();

        } catch (err) {

          callback(err);

        }

      })();

but that gets me

FATAL ERROR: AsyncShutdown timeout in profile-before-change Conditions: [{"name":"Sqlite.jsm shutdown blocker","state":{"description":"Waiting for connections to close","state":[{"name":"betterbibtex-lokijs.sqlite#0: waiting for shutdown","state":{"identifier":"betterbibtex-lokijs.sqlite#0","isCloseRequested":false,"hasDbConn":true,"hasInProgressTransaction":false,"pendingStatements":0,"statementCounter":8},"filename":"resource://gre/modules/Sqlite.jsm","lineNumber":259,"stack":["resource://gre/modules/Sqlite.jsm:ConnectionData:259","resource://gre/modules/Sqlite.jsm:OpenedConnection:1122","resource://gre/modules/Sqlite.jsm:openConnection/</<:933"]}]},"filename":"resource://gre/modules/Sqlite.jsm","lineNumber":157,"stack":["resource://gre/modules/Sqlite.jsm:null:157","resource://gre/modules/XPCOMUtils.jsm:XPCU_defineLazyGetter/<.get:198","resource://gre/modules/Sqlite.jsm:ConnectionData:259","resource://gre/modules/Sqlite.jsm:OpenedConnection:1122","resource://gre/modules/Sqlite.jsm:openConnection/</<:933"]}] At least one completion condition failed to complete within a reasonable amount of time. Causing a crash to ensure that we do not leave the user with an unresponsive process draining resources. 

followed by the Mozilla crash reporter popping up.

You can't safely run async code in a shutdown listener, because Zotero.shutdown() doesn't wait for promises from listeners. If it did, things would probably work as expected when switching from full mode to connector mode (which is going away anyway), because Zotero.shutdown() would wait to close the DB until your queries had finished, but there's a good chance it would cause problems at application shutdown, since Zotero.shutdown() is triggered by an observer for the XPCOM quit-application event, and the XPCOM observer system doesn't wait for promises.

Also:

1) It's still not clear that you're using Zotero.Promise.coroutine() correctly. It should almost never be necessary to use the Z.P.coroutine(function* { ...})() syntax. Even if the shutdown code waited for promises from listeners, you'd want to simply wrap the shutdown listener with Z.P.coroutine(). From your code above, unless you didn't copy a 'yield' from the coroutine() line or the callback is resolving a promise that's returned below (which would be an awkward way of doing things), your shutdown listener isn't actually waiting for the generator function to complete before returning.

2) You don't need parentheses around 'yield …' when it's the only thing on the line, only when it's used as a parameter.

[Emiliano Heyns]

On Monday, July 4, 2016 at 9:44:53 AM UTC+2, Dan Stillman wrote:

On 7/3/16 4:53 PM, Emiliano Heyns wrote:

You can't safely run async code in a shutdown listener, because Zotero.shutdown() doesn't wait for promises from listeners. If it did, things would probably work as expected when switching from full mode to connector mode (which is going away anyway), because Zotero.shutdown() would wait to close the DB until your queries had finished, but there's a good chance it would cause problems at application shutdown, since Zotero.shutdown() is triggered by an observer for the XPCOM quit-application event, and the XPCOM observer system doesn't wait for promises.

OK, so that means I structurally can't save things at shutdown? That's going to be a real issue for me. I'll see whether I can work around this, but I keep most of my stuff in an in-memory database now, and I'd prefer to be sure I have flushed everything to disk before shutdown.

Also:

1) It's still not clear that you're using Zotero.Promise.coroutine() correctly. It should almost never be necessary to use the Z.P.coroutine(function* { ...})() syntax. Even if the shutdown code waited for promises from listeners, you'd want to simply wrap the shutdown listener with Z.P.coroutine().

This code is part of my lokijs save/load adapter. It gets called by lokijs, and lokijs expects its calback to be called when done. I'm open to any code structure that gets this done -- I just figured this was the way to do it. The bluebird documentation seems a little focused on the everything-uses-bluebird case -- in my case, I need to meander between promises, syncronous code and callback-based code, and I simply haven't found much that explains how these should interact.

 

From your code above, unless you didn't copy a 'yield' from the coroutine() line or the callback is resolving a promise that's returned below (which would be an awkward way of doing things), your shutdown listener isn't actually waiting for the generator function to complete before returning.

Ah. That makes sense, I suppose, but I'd need to dive into how LokiJS expects this to be set up.

 

2) You don't need parentheses around 'yield …' when it's the only thing on the line, only when it's used as a parameter.

That's just a side effect from the coffeescript compilation.

[Emiliano Heyns]

Is there a signal I can wait for to know the stock translators have been loaded? During the loading of the BBT translators I call Zotero.Translators.get(translatorID), and I get "Error: Translators not yet loaded"

[Dan Stillman]

You can yield on Zotero.Translators.init(), which is a no-op if they're
already loaded.

[Emiliano Heyns]

On Wednesday, July 6, 2016 at 7:29:50 PM UTC+2, Dan Stillman wrote:

You can yield on Zotero.Translators.init(), which is a no-op if they're
already loaded.

That gets me a little further, but I'm now getting

Error(s) encountered during statement execution: no such column: fileName [QUERY: SELECT fileName, metadataJSON, lastModifiedTime FROM translatorCache

The error doesn't have a stack trace, so I don't know where it originates exactly, but I'm assuming it's from calling Zotero.Translators.init(). Should I be waiting/can I wait for Zotero to have started up and "settled" before initializing BBT? I have a sense a lot of the issues I'm going to hit in porting BBT will be prevented by waiting for that.

On another topic, a lot of the things I'm going to be stumbling into are bound to be issues arising from me monkey-patching Zotero left and right. This is going to be harder (and some would say this is a good thing) with the pervasive use of coroutines, which usually wraps anonymous functions, safely out of reach of BBT.

Rather than trying to find new ways to monkey-patch stuff under 5.x, I'd much prefer it if there could be clean APIs (or fixes in some occasions) to make these monkey-patches obsolete; could you have a look at my list of monkey-patches below to see if any of them would qualify? Trigger warning: absolutely everything listed below is an ugly hack, bound to offend the sensibilities of most sensible software engineers. I don't do these for the mere fun of it, and try to avoid monkey-patching if any other route is available. Some of these may in fact already have APIs that have popped up after I implemented these hacks -- I must acknowledge I haven't checked whether this is the case.

1. Zotero.Sync.Storage.processDownload. I wrap this method, calling it without changes, but add a timeout a second after it fires to emulate an observer on 'item' firing. It currently calls my own notifier handler function directly, but could in principle just activate any registered item-change handler. I do this because the notification for webdav-synced attachments fires before the attachment is unpacked from its zip, this delayed notification will allow me to handle the updated attachment as usual.
   
2. Zotero.Server.DataListener.prototype._generateResponse / Zotero.Server.DataListener.prototype._requestFinished. Before anyone freaks out: I'm not proposing to keep anything even resembling what I did, but the reason I wrapped this is so I could return promises for strings rather than simply strings using the sendResponseCallback. This would seem be a good philosophical fit for 5.0 to replace the callback with a promise-based approach. My current wrapper looks whether the response is a promise, and acts on that if so, and passes on all arguments unchanged if the response is a string
3. Zotero.Search.prototype.save. I wrap this method to be notified of changes to a search in order to schedule auto-exports of saved searches.
   
4. Zotero.ItemTreeView.prototype.getCellText. This is one I'd love to get rid off. Currently, it looks if the cell being requested is for the 'extra' column, and if the relevant setting is enabled in BBT, it returns the citation key instead of the text that would normally be looked up. If both these conditions do not apply, I simply call the original method. This is for sure a rather ugly hack, but Zotero has loads of places where it validates that a "valid" column is being shown, and of course "citekey" is not one of the approved fields for any time, so this is the only thing I could think of. I'd very much prefer there to be a clean way for extensions to declare a new column type.
   
5. Zotero.Translate.Export.prototype.translate. Another one I'd love to ditch. I wrap this method to get more info available on the translation that's about to start. I note what's about to be exported (saved search, collection, library or "other") so I can register that for auto-export if that option has been enabled for the current export. I note the folder being exported to (although this is really just for a minor BBT feature, I could endure its loss)
   
6. Zotero.Translate.Export.prototype._prepareTranslation. Marks this._itemGetter to note whether file export has been requested. This is only used in nextItem (see following point)
   
7. Zotero.Translate.ItemGetter.prototype.nextItem. The reason for wrapping this method may have become obsolete in 5.0. In 4.0, pre-translation reference serialization took the bulk of the time of the translation -- according to my highly unscientific tests, some 80% of the export time was spent in serializing references, I'd venture to guess because it involved SQL calls, which have high per-call overhead. This was especially problematic for auto-exports. What this wrapper does is cache the serialization result to memory and directly return that if a hit was found. This was only done under two conditions: the running export was for a BBT translator, and no file export had been requested. For any other case, the original method is called unchanged. The associated cache was invalidated when a reference changed, yada yada. Given that 5.0 appears to have most of the references in memory in any case, this may no longer be relevant, but at least in 4.0 it is a real performance drag on exports -- all translators would benefit from this cache, but I didn't dare to expose non-BBT translators to it because I wasn't sure about side effects.
   
8. ZoteroPane_Local.buildCollectionContextMenu. Wrapped in order to add menu entries to the collection right-click menu.

[Dan Stillman]

On 7/7/16 10:03 AM, Emiliano Heyns wrote:

On Wednesday, July 6, 2016 at 7:29:50 PM UTC+2, Dan Stillman wrote:

You can yield on Zotero.Translators.init(), which is a no-op if they're
already loaded.

That gets me a little further, but I'm now getting

Error(s) encountered during statement execution: no such column: fileName [QUERY: SELECT fileName, metadataJSON, lastModifiedTime FROM translatorCache

The error doesn't have a stack trace, so I don't know where it originates exactly, but I'm assuming it's from calling Zotero.Translators.init(). Should I be waiting/can I wait for Zotero to have started up and "settled" before initializing BBT? I have a sense a lot of the issues I'm going to hit in porting BBT will be prevented by waiting for that.

Yeah, you shouldn't do anything before Zotero.initializationPromise resolves. You can add a 'yield Zotero.initializationPromise;' to your startup code.

You can also yield on Zotero.schemaUpdatePromise to wait for the built-in translators to be updated from the repo, which happens about a second after initialization.

Rather than trying to find new ways to monkey-patch stuff under 5.x, I'd much prefer it if there could be clean APIs (or fixes in some occasions) to make these monkey-patches obsolete; could you have a look at my list of monkey-patches below to see if any of them would qualify?

Will get back to you on these.

[Emiliano Heyns]

On Friday, July 8, 2016 at 8:11:26 AM UTC+2, Dan Stillman wrote:

Yeah, you shouldn't do anything before Zotero.initializationPromise resolves. You can add a 'yield Zotero.initializationPromise;' to your startup code.

You can also yield on Zotero.schemaUpdatePromise to wait for the built-in translators to be updated from the repo, which happens about a second after initialization.

That sounds good.

I am used to initializing BBT from the overlay loader code. This code does not run in the context of a generator function, so I don't think calling "yield" there will do the job. You previously mentioned I shouldn't do something like

Zotero.Promise.coroutine(function* () {

   yield Zotero.schemaUpdatePromise();

  // do my init stuff here

})

but I have looked and I really can't find any other way to kick this off.

 

Rather than trying to find new ways to monkey-patch stuff under 5.x, I'd much prefer it if there could be clean APIs (or fixes in some occasions) to make these monkey-patches obsolete; could you have a look at my list of monkey-patches below to see if any of them would qualify?

Will get back to you on these.

Super, I really appreciate that.

I forgot to mention this one:

1. ZoteroItemPane.viewItem. Wrapped so I am notified of a new reference being displayed in the itemPane so I can show its citation key. Wrapping this doesn't work under 5.x as it's no longer a function but a coroutine. I'm tinkering with a MutationObserver on 'dynamic-fields' but the results are spotty so far.

[Emiliano Heyns]

On Friday, July 8, 2016 at 8:11:26 AM UTC+2, Dan Stillman wrote:

The error doesn't have a stack trace, so I don't know where it originates exactly, but I'm assuming it's from calling Zotero.Translators.init(). Should I be waiting/can I wait for Zotero to have started up and "settled" before initializing BBT? I have a sense a lot of the issues I'm going to hit in porting BBT will be prevented by waiting for that.

Yeah, you shouldn't do anything before Zotero.initializationPromise resolves. You can add a 'yield Zotero.initializationPromise;' to your startup code.

Hey, you say Zotero.Translators.init() is a no-op after the first time it's done its job? I used to call Zotero.Translators.init() after I saved my translators in the appropriate directory, will this mean my call to Zotero.Translators.init() will not pick them up anymore (being a no-op at that time)?

[Dan Stillman]

On 7/8/16 3:15 AM, Emiliano Heyns wrote:
> I am used to initializing BBT from the overlay loader code. This code
> does not run in the context of a generator function, so I don't think
> calling "yield" there will do the job. You previously mentioned I
> shouldn't do something like
>
> Zotero.Promise.coroutine(function* () {
> yield Zotero.schemaUpdatePromise();
> // do my init stuff here
> })
>
> but I have looked and I really can't find any other way to kick this off.

You can wrap any function with coroutine(), even if the calling code
doesn't handle promises. If you want to use yield somewhere, that's what
you do.

We pretty much only use Zotero.Promise.coroutine(...)() in XBL bindings,
where functions are defined in XML and there's no way to wrap them with
coroutine(). (There are a few other cases that are mostly left over from
before we had coroutine(), or where the promises handlers are
essentially functioning as GOTOs and would have to be rewritten.)

[Dan Stillman]

On 7/8/16 3:24 AM, Emiliano Heyns wrote:
> Hey, you say Zotero.Translators.init() is a no-op after the first time
> it's done its job? I used to call Zotero.Translators.init() after I
> saved my translators in the appropriate directory, will this mean my
> call to Zotero.Translators.init() will not pick them up anymore (being
> a no-op at that time)?

Zotero.Translators.reinit()

[Emiliano Heyns]

On Friday, July 8, 2016 at 9:15:04 AM UTC+2, Emiliano Heyns wrote:

On Friday, July 8, 2016 at 8:11:26 AM UTC+2, Dan Stillman wrote:

Yeah, you shouldn't do anything before Zotero.initializationPromise resolves. You can add a 'yield Zotero.initializationPromise;' to your startup code.

You can also yield on Zotero.schemaUpdatePromise to wait for the built-in translators to be updated from the repo, which happens about a second after initialization.

That sounds good.

I am used to initializing BBT from the overlay loader code. This code does not run in the context of a generator function, so I don't think calling "yield" there will do the job. You previously mentioned I shouldn't do something like

Zotero.Promise.coroutine(function* () {

   yield Zotero.schemaUpdatePromise();

  // do my init stuff here

})()

I've tried "yield Zotero.schemaUpdatePromise()", which nets me "schemaUpdatePromise is not a function", and "yield Zotero.schemaUpdatePromise", which gets me "TypeError: A value undefined was yielded that could not be treated as a promise". Should I first yield on "initializationPromise", and then schemaUpdatePromise?

[Dan Stillman]

Sorry, Zotero.Schema.schemaUpdatePromise.

[Emiliano Heyns]

On Thursday, July 7, 2016 at 4:03:41 PM UTC+2, Emiliano Heyns wrote:

Rather than trying to find new ways to monkey-patch stuff under 5.x, I'd much prefer it if there could be clean APIs (or fixes in some occasions) to make these monkey-patches obsolete; could you have a look at my list of monkey-patches below to see if any of them would qualify? Trigger warning: absolutely everything listed below is an ugly hack, bound to offend the sensibilities of most sensible software engineers. I don't do these for the mere fun of it, and try to avoid monkey-patching if any other route is available. Some of these may in fact already have APIs that have popped up after I implemented these hacks -- I must acknowledge I haven't checked whether this is the case.

1. Zotero.Sync.Storage.processDownload. I wrap this method, calling it without changes, but add a timeout a second after it fires to emulate an observer on 'item' firing. It currently calls my own notifier handler function directly, but could in principle just activate any registered item-change handler. I do this because the notification for webdav-synced attachments fires before the attachment is unpacked from its zip, this delayed notification will allow me to handle the updated attachment as usual.
   
2. Zotero.Server.DataListener.prototype._generateResponse / Zotero.Server.DataListener.prototype._requestFinished. Before anyone freaks out: I'm not proposing to keep anything even resembling what I did, but the reason I wrapped this is so I could return promises for strings rather than simply strings using the sendResponseCallback. This would seem be a good philosophical fit for 5.0 to replace the callback with a promise-based approach. My current wrapper looks whether the response is a promise, and acts on that if so, and passes on all arguments unchanged if the response is a string
3. Zotero.Search.prototype.save. I wrap this method to be notified of changes to a search in order to schedule auto-exports of saved searches.
   
4. Zotero.ItemTreeView.prototype.getCellText. This is one I'd love to get rid off. Currently, it looks if the cell being requested is for the 'extra' column, and if the relevant setting is enabled in BBT, it returns the citation key instead of the text that would normally be looked up. If both these conditions do not apply, I simply call the original method. This is for sure a rather ugly hack, but Zotero has loads of places where it validates that a "valid" column is being shown, and of course "citekey" is not one of the approved fields for any time, so this is the only thing I could think of. I'd very much prefer there to be a clean way for extensions to declare a new column type.
   
5. Zotero.Translate.Export.prototype.translate. Another one I'd love to ditch. I wrap this method to get more info available on the translation that's about to start. I note what's about to be exported (saved search, collection, library or "other") so I can register that for auto-export if that option has been enabled for the current export. I note the folder being exported to (although this is really just for a minor BBT feature, I could endure its loss)
   
6. Zotero.Translate.Export.prototype._prepareTranslation. Marks this._itemGetter to note whether file export has been requested. This is only used in nextItem (see following point)
   
7. Zotero.Translate.ItemGetter.prototype.nextItem. The reason for wrapping this method may have become obsolete in 5.0. In 4.0, pre-translation reference serialization took the bulk of the time of the translation -- according to my highly unscientific tests, some 80% of the export time was spent in serializing references, I'd venture to guess because it involved SQL calls, which have high per-call overhead. This was especially problematic for auto-exports. What this wrapper does is cache the serialization result to memory and directly return that if a hit was found. This was only done under two conditions: the running export was for a BBT translator, and no file export had been requested. For any other case, the original method is called unchanged. The associated cache was invalidated when a reference changed, yada yada. Given that 5.0 appears to have most of the references in memory in any case, this may no longer be relevant, but at least in 4.0 it is a real performance drag on exports -- all translators would benefit from this cache, but I didn't dare to expose non-BBT translators to it because I wasn't sure about side effects.
   
8. ZoteroPane_Local.buildCollectionContextMenu. Wrapped in order to add menu entries to the collection right-click menu.

Any news on these? I'd have to do some really ugly stuff to get this done in 5.0, and I'd really rather not. In 4.0, monkey-patching is simple and can be done fairly safely when diligent, but in 5.0 (or specifically, in any bluebird-heavy code), it's really hard to do targeted monkey-patches and I'd end up doing one mega-monkey-patch and trying to derive from context what actual behavior to patch... it's not pretty, and the code makes me very nervous. 

[Dan Stillman]

On 8/7/16 5:56 AM, Emiliano Heyns wrote:

Any news on these? I'd have to do some really ugly stuff to get this done in 5.0, and I'd really rather not. In 4.0, monkey-patching is simple and can be done fairly safely when diligent, but in 5.0 (or specifically, in any bluebird-heavy code), it's really hard to do targeted monkey-patches and I'd end up doing one mega-monkey-patch and trying to derive from context what actual behavior to patch... it's not pretty, and the code makes me very nervous.

Not that I'm a fan of monkey-patching, but can you explain the difficulty of monkey-patching in 5.0? If you want to override a coroutine, you just need to 1) consume the same arguments, 2) wait on a promise that the original returns if you need to do something else with it, and 3) return another promise. Is there something specific that's posing a problem?

I think there are some more general ways we could greatly improve extensibility, but realistically I don't see that happening until we're on Electron/React.

But a couple easy ones (I think) from your list:

Zotero.Sync.Storage.processDownload. I wrap this method, calling it without changes, but add a timeout a second after it fires to emulate an observer on 'item' firing. It currently calls my own notifier handler function directly, but could in principle just activate any registered item-change handler. I do this because the notification for webdav-synced attachments fires before the attachment is unpacked from its zip, this delayed notification will allow me to handle the updated attachment as usual.

Not sure if this is still a problem in 5.0, but I'd think you could do this with a normal notifier observer and just delay if it's an attachment in a WebDAV library (Zotero.Sync.Storage.Local.getModeForLibrary()).

Zotero.Search.prototype.save. I wrap this method to be notified of changes to a search in order to schedule auto-exports of saved searches.

You should be able to use 'search' notifier events for this.

[Emiliano Heyns]

On Thursday, August 11, 2016 at 8:09:17 AM UTC+2, Dan Stillman wrote:

On 8/7/16 5:56 AM, Emiliano Heyns wrote:

Any news on these? I'd have to do some really ugly stuff to get this done in 5.0, and I'd really rather not. In 4.0, monkey-patching is simple and can be done fairly safely when diligent, but in 5.0 (or specifically, in any bluebird-heavy code), it's really hard to do targeted monkey-patches and I'd end up doing one mega-monkey-patch and trying to derive from context what actual behavior to patch... it's not pretty, and the code makes me very nervous.

Not that I'm a fan of monkey-patching, but can you explain the difficulty of monkey-patching in 5.0?

Not a fan perse myself, but fascinated with what it can do. I use it when no other means are present (or known to me, as you point out below).

The difficulty of monkey-patching in 5.0 is not with named functions but with anonymous functions (if you grep for "then.*function" you find some of them). This is a common pattern with promises as far as I can tell, but patching them, while not impossible, is incredibly fragile.

 

I think there are some more general ways we could greatly improve extensibility, but realistically I don't see that happening until we're on Electron/React.

Ah, pity. Is there a timeline for this? As in, could I sit out 5.0?

 

But a couple easy ones (I think) from your list:

Zotero.Sync.Storage.processDownload. I wrap this method, calling it without changes, but add a timeout a second after it fires to emulate an observer on 'item' firing. It currently calls my own notifier handler function directly, but could in principle just activate any registered item-change handler. I do this because the notification for webdav-synced attachments fires before the attachment is unpacked from its zip, this delayed notification will allow me to handle the updated attachment as usual.

Not sure if this is still a problem in 5.0, but I'd think you could do this with a normal notifier observer and just delay if it's an attachment in a WebDAV library (Zotero.Sync.Storage.Local.getModeForLibrary()).

Ah, I didn't know this. But this was actually one I had hoped might just be changed in Zotero. If the item notifier is supposed to fire after an attachment change, shouldn't it also fire after the attachment was unpacked from the zip? It does after all change (again) at that point.

 

Zotero.Search.prototype.save. I wrap this method to be notified of changes to a search in order to schedule auto-exports of saved searches.

You should be able to use 'search' notifier events for this.

 OK, so that fires when the search is changed? Nice.

[Dan Stillman]

On 8/11/16 7:39 AM, Emiliano Heyns wrote:

The difficulty of monkey-patching in 5.0 is not with named functions but with anonymous functions (if you grep for "then.*function" you find some of them). This is a common pattern with promises as far as I can tell, but patching them, while not impossible, is incredibly fragile.

Could you give specific examples where you want to do this? coroutines()/generators are mostly an alternative (wrapper, technically) to the .then callback model of promises, so it's mostly either in code that hasn't been updated to use generators or functioning as a glorified goto to avoid some large ifs. Either way, many existing instances could probably be rewritten. But I would also think that in most places just using the value of the final promise would be sufficient.

 

I think there are some more general ways we could greatly improve extensibility, but realistically I don't see that happening until we're on Electron/React.

Ah, pity. Is there a timeline for this? As in, could I sit out 5.0?

Sometime next year. But the Electron rewrite will take some work, so people will definitely be using 5.0 in the interim.

Zotero.Sync.Storage.processDownload. I wrap this method, calling it without changes, but add a timeout a second after it fires to emulate an observer on 'item' firing. It currently calls my own notifier handler function directly, but could in principle just activate any registered item-change handler. I do this because the notification for webdav-synced attachments fires before the attachment is unpacked from its zip, this delayed notification will allow me to handle the updated attachment as usual.

Not sure if this is still a problem in 5.0, but I'd think you could do this with a normal notifier observer and just delay if it's an attachment in a WebDAV library (Zotero.Sync.Storage.Local.getModeForLibrary()).

Ah, I didn't know this. But this was actually one I had hoped might just be changed in Zotero. If the item notifier is supposed to fire after an attachment change, shouldn't it also fire after the attachment was unpacked from the zip? It does after all change (again) at that point.

If you can provide example code that demonstrates the problem, I can take a look.

[Emiliano Heyns]

On Thursday, August 11, 2016 at 5:52:40 PM UTC+2, Dan Stillman wrote:

On 8/11/16 7:39 AM, Emiliano Heyns wrote:

The difficulty of monkey-patching in 5.0 is not with named functions but with anonymous functions (if you grep for "then.*function" you find some of them). This is a common pattern with promises as far as I can tell, but patching them, while not impossible, is incredibly fragile.

Could you give specific examples where you want to do this? coroutines()/generators are mostly an alternative (wrapper, technically) to the .then callback model of promises, so it's mostly either in code that hasn't been updated to use generators or functioning as a glorified goto to avoid some large ifs. Either way, many existing instances could probably be rewritten. But I would also think that in most places just using the value of the final promise would be sufficient.

I've looked, but I must have been mistaken. All the places where I now monkey-patch are still just named functions. Assuming `this` has the right value for the context (and it sure looks like it), it looks like nothing will change in this regard. Sorry for the noise.

 

Not sure if this is still a problem in 5.0, but I'd think you could do this with a normal notifier observer and just delay if it's an attachment in a WebDAV library (Zotero.Sync.Storage.Local.getModeForLibrary()).

Ah, I didn't know this. But this was actually one I had hoped might just be changed in Zotero. If the item notifier is supposed to fire after an attachment change, shouldn't it also fire after the attachment was unpacked from the zip? It does after all change (again) at that point.

If you can provide example code that demonstrates the problem, I can take a look.

It's a little hard to get code that reliably demonstrates it, as it's timing sensitive, but this behavior is what triggered it for me:

var itemObserver = {

  notify: (function(event, type, ids, extraData) {

    if (!(type === 'item' && (event === 'add' || event === 'modify'))) {

      return;

    }

    /*

      Zotero seems to syncs WebDAV-hosted attachments in two stages:

      1. Get the metadata from the Zotero Server

      2. Get the attachment data from WebDAV

      however Zotero tells extensions that the item has been updated after 1, but not (again) after 2. So I when I kick

      off an auto-export while the attachment wasn't yet really on disk, during export, Zotero would tell me "this is not really an attached

      file", and throw an error.

     */

    var translation = new Zotero.Translate.Export();

    translation.setItems(Zotero.Items.get(ids));

    translation.setTranslator("9cb70025-a888-4a29-a210-93ec52da40d4");

    var path = Components.classes["@mozilla.org/file/local;1"].createInstance(Components.interfaces.nsILocalFile);

    path.initWithPath('/tmp/test.bib');

    translation.setLocation(path);

    translation.setHandler('done', function(obj, success) {

      Zotero.debug(obj, success);

    });

    translation.translate();

  })

};

Zotero.Notifier.registerObserver(itemObserver, ['item']); 

[Emiliano Heyns]

But if I understand you correctly, I could at least just ignore all events for attachments that I know are on webdav, and just schedule a faux item notification event for my itemObserver handler via a timeOut. Or rather, dispatch such a faux event after processDownload, because it takes away the time-sensitivty. 

[Emiliano Heyns]

Can you explain how Zotero.Schema.schemaUpdatePromise works, technically? It looks like it enables any number of places to wait on the promise. I could use something like this to do dependent loads in BBT, e.g. I'd want parts of the BBT code to only proceed after I've loaded the BBT database.

[Emiliano Heyns]

The port really is doing my head in. Can you describe the process that got you guys through promisification? Did you rewrite from the ground up (tried that, not fun), simply wrapped coroutine around each method and called it using yield (tried but kept losing track of where I had to modify the caller), some other method to the madness?

Porting BBT is going to be quite a bit of work from the looks of it, I had underestimated how much code is there now as it grew incrementally, and I feel wholly overwhelmed by the prospect of the port right now. I've had a few false starts, but I really can't put it off any longer. Insights on how you guys managed will be helpful.

[Adomas Venčkauskas]

simply wrapped coroutine around each method and called it using yield (tried but kept losing track of where I had to modify the caller)

Yeah, that is the basic premise. An IDE is helpful in identifying all callers of a function. So there was

function foo(bar) {

doAsyncThing(bar, function() {

Zotero.debug("after async");

});

}

Now instead is

var foo = Zotero.Promise.coroutine(function* (bar) {

yield doAsyncThing(bar);

Zotero.debug("after async");

})

And to promisify native XPCOM calls like statment.executeAsync we went from

statement.executeAsync({

handleResult: function(result) {

    Zotero.debug("result: " + result.getNextRow().getResultByIndex(0))

},

handleError: function(e) {

Zotero.debug(e.message);

}

});

to

// Promisified previous async call

function doDb(statement) {

let deferred = Zotero.Promise.defer();

statement.executeAsync({

// resolves promise with the result

handleResult: deferred.resolve

// rejects promise with the error

handleError: deferred.reject

});

// returns an unresolved promise

return deferred.promise;

}

// Somewhere in a coroutine

try {

var result = yield doDb(statement);

Zotero.debug("result: " + result.getNextRow().getResultByIndex(0))

}

catch (e) {

Zotero.debug(e.message);

}

So deferred.resolve() and deferred.reject() are used to, well, resolve and reject the promise returned synchronously from a function.

Obviously, there are places at which promises or coroutines are not yielded, just like with callback based code, where you wouldn't provide a callback.

[Emiliano Heyns]

On Wednesday, October 5, 2016 at 9:05:31 PM UTC+2, Adomas Venčkauskas wrote:

simply wrapped coroutine around each method and called it using yield (tried but kept losing track of where I had to modify the caller)

Yeah, that is the basic premise. An IDE is helpful in identifying all callers of a function. So there was

I don't know of an IDE that does coffeescript refactoring. Maybe it's time to for me to give up coffeescript.

My code relies crucially on certain parts having executed in full before anything else happens. I am hoping that the mechanism that's used for the schemaupdatepromise will allow me to emulate that. It looks like that promise is created once, but it's then called/used/executed (I don't know exactly which of these apply here) at multiple places, and it becomes a no-op once its done its work (I think?), but I don't grasp how it does all this.

Not super stoked about this port to be honest, but it will have to be done. 

[Dan Stillman]

On 10/6/16 6:05 PM, Emiliano Heyns wrote:

On Wednesday, October 5, 2016 at 9:05:31 PM UTC+2, Adomas Venčkauskas wrote:

simply wrapped coroutine around each method and called it using yield (tried but kept losing track of where I had to modify the caller)

Yeah, that is the basic premise. An IDE is helpful in identifying all callers of a function. So there was

I don't know of an IDE that does coffeescript refactoring. Maybe it's time to for me to give up coffeescript.

It's really just about grepping the code for the function you're changing — that's all I do, at least.

My code relies crucially on certain parts having executed in full before anything else happens. I am hoping that the mechanism that's used for the schemaupdatepromise will allow me to emulate that. It looks like that promise is created once, but it's then called/used/executed (I don't know exactly which of these apply here) at multiple places, and it becomes a no-op once its done its work (I think?), but I don't grasp how it does all this.

Yes, basically. You can await the result of a promise from multiple places, and as soon as the promise is resolved (or if it's already resolved) all such places will receive the same resolution value or rejection. (Behind the scenes, these are just additional callbacks, and if the promise is resolved the callbacks get called immediately.) This isn't really a standard use of promises (and some people would probably argue it's an anti-pattern), but it works well enough for our purposes.

[Emiliano Heyns]

On Friday, October 7, 2016 at 12:12:49 AM UTC+2, Dan Stillman wrote:

My code relies crucially on certain parts having executed in full before anything else happens. I am hoping that the mechanism that's used for the schemaupdatepromise will allow me to emulate that. It looks like that promise is created once, but it's then called/used/executed (I don't know exactly which of these apply here) at multiple places, and it becomes a no-op once its done its work (I think?), but I don't grasp how it does all this.

Yes, basically. You can await the result of a promise from multiple places, and as soon as the promise is resolved (or if it's already resolved) all such places will receive the same resolution value or rejection. (Behind the scenes, these are just additional callbacks, and if the promise is resolved the callbacks get called immediately.) This isn't really a standard use of promises (and some people would probably argue it's an anti-pattern), but it works well enough for our purposes.

So this is just standard promises behavior, but an atypical use? Resolving a promise somehow signals all its callers in any case?

I do not understand promises.

[Emiliano Heyns]

On Friday, October 7, 2016 at 12:23:34 AM UTC+2, Emiliano Heyns wrote:

Yes, basically. You can await the result of a promise from multiple places, and as soon as the promise is resolved (or if it's already resolved) all such places will receive the same resolution value or rejection. (Behind the scenes, these are just additional callbacks, and if the promise is resolved the callbacks get called immediately.) This isn't really a standard use of promises (and some people would probably argue it's an anti-pattern), but it works well enough for our purposes.

So this is just standard promises behavior, but an atypical use? Resolving a promise somehow signals all its callers in any case?

I'm looking at http://stackoverflow.com/questions/17718673/how-is-a-promise-defer-library-implemented to try to understand how they work, and I see now promises do maintain an internal list of callers, but I still don't understand (from this perhaps too simple explanation) how callbacks added by an invocation of "then" would be called if these are added if the promise was resolved before that point. 

[Emiliano Heyns]

Ah wait -- a promise knows it's state and can just call synchronously after resolution. OK, that takes care of the order-of-execution bits.

[Dan Stillman]

On 10/6/16 6:34 PM, Emiliano Heyns wrote:
> Ah wait -- a promise knows it's state and can just call synchronously
> after resolution. OK, that takes care of the order-of-execution bits.

Technically not synchronously — the handler always fires in a future
turn of the event loop, even if it's already resolved. E.g.,

var p = Zotero.Promise.resolve();
p.then(() => Zotero.debug(2));
Zotero.debug(1);

will return 1 before 2. But it won't need to wait more than that.

[Emiliano Heyns]

Right -- because if the "then" function takes a long time, it's not supposed to run to completion before the "1" line fires. Got it. 

OK, that's promises explained then. 

[Emiliano Heyns]

Can bluebird as it is used in Zotero to be configured to log to the Zotero debug log instead of to console.log? I'm getting "Unhandled rejection error" in my trials which would be very handy to find in the Zotero log (preferably with a location).

[Emiliano Heyns]

What is the 5.0 equivalent of "Zotero.getCollections()"?

[Adomas Venčkauskas]

As described here, the calls have to be replaced with Zotero.Collections.getByLibrary(). 

[Emiliano Heyns]

On Friday, October 14, 2016 at 10:22:29 AM UTC+2, Adomas Venčkauskas wrote:

As described here, the calls have to be replaced with Zotero.Collections.getByLibrary(). 

And the default library is 0 rather than null in 5.0, correct?

[Emiliano Heyns]

On Friday, October 14, 2016 at 10:22:29 AM UTC+2, Adomas Venčkauskas wrote:

As described here, the calls have to be replaced with Zotero.Collections.getByLibrary(). 

And does this function return an array, or a promise for an array? Nothing in the signature indicates it's a promise, but as I assume it will touch the DB, it sort of has to be in 5.0

[Emiliano Heyns]

On Friday, October 14, 2016 at 10:22:29 AM UTC+2, Adomas Venčkauskas wrote:

As described here, the calls have to be replaced with Zotero.Collections.getByLibrary(). 

Is this correct? The way I read the code it only accepts a libraryID.

 

[Adomas Venčkauskas]

As per this update, you should now use Zotero.Libraries.userLibraryID to get the user library ID.

--
You received this message because you are subscribed to a topic in the Google Groups "zotero-dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/zotero-dev/naAxXIbpDhU/unsubscribe.
To unsubscribe from this group and all its topics, send an email to zotero-dev+unsubscribe@googlegroups.com.
To post to this group, send email to zoter...@googlegroups.com.
Visit this group at https://groups.google.com/group/zotero-dev.
For more options, visit https://groups.google.com/d/optout.

--

Software engineer, computer scientist, music enthusiast

http://adomasven.com

[Emiliano Heyns]

I'm getting what appears to be an error in my test log ("Tags not yet loaded"), but no indication of the place that triggered this.

[Adomas Venčkauskas]

Yeah, don't worry about that one. It's a Zotero tag selector and feed problem. Now fixed.

[Dan Stillman]

It returns an array as documented. (You can also just Zotero.debug() the response to see this.) Collections are loaded at startup, hence the lack of a promise — this is "deasyncification".

The "parentID" reference in the comment is outdated, though. (That code was moved below to _getByContainer().)

[Emiliano Heyns]

On Wednesday, October 5, 2016 at 11:25:54 AM UTC+2, Emiliano Heyns wrote:

Can you explain how Zotero.Schema.schemaUpdatePromise works, technically? It looks like it enables any number of places to wait on the promise. I could use something like this to do dependent loads in BBT, e.g. I'd want parts of the BBT code to only proceed after I've loaded the BBT database.

What is the state of the database after schemaUpdatePromise resolves? I know some things are in memory after startup (like collections) but are (parts of) the references also in memory after this resolves?

[Dan Stillman]

Collections and searches from all libraries should be loaded, and items from the currently selected library should be loaded. Items from other libraries won't be loaded. (Specifically, in Standalone, schema updating waits for Zotero.uiReadyPromise, which is resolved once items for the current library are loaded.)

If you need items that might not be loaded, you can use Zotero.Items.getAsync() (or getByLibraryAndKeyAsync) to get them. When you know that they're loaded, the synchronous versions are better.

[Emiliano Heyns]

Damn, I had hoped all items would be loaded. Most of the time this isn't going to be an issue (I think...) but auto-exports may be active on libraries other than the currently selected library. This may become problematic as I call Zotero.Utilities.Internal.itemToExportFormat in the chain that gets kicked off for auto-export, and I'm going to assume this is not safe to call on an item that isn't loaded into memory.

--

[Emiliano Heyns]

Is there an async version of itemtoexportformat? 

[Dan Stillman]

On 2/19/17 11:22 AM, Emiliano Heyns wrote:
> Is there an async version of itemtoexportformat?

No, but you can do something similar to this to load all items in the
library:

https://github.com/zotero/zotero/blob/bb0fa7389971ba7f979809ba7971d48a01edf0ac/chrome/content/zotero/zoteroPane.js#L1256

Or if it's not all items, you can call Zotero.Items.loadDataTypes(items,
['item']) on the specific items you need.

[Emiliano Heyns]

On Monday, June 13, 2016 at 10:02:17 AM UTC+2, Dan Stillman wrote:

On 6/13/16 3:39 AM, Emiliano Heyns wrote:

1. Can I add my own tests, and can I select which tests to run (I'd rather not run the whole Zotero test suite for my own unit tests), and

Well, you can add files to test/tests/ locally. If you add fooTest.js, you can then run tests in that file with `./test/runtests.sh foo` (or multiple separated by commas), or `./test/runtests.sh -g 'test title to search for' foo` to run specific tests.

If you're not testing something translator-related, it's much quicker to pass -b to skip bundled file installation. -d 5 will display debug output. See ./test/runtests.sh -h for more.

I'm looking at Zotero.Utilities.Internal.itemToExportFormat, but I don't see where the async happens there. itemToExportFormat uses item.getField, which must be async (right?) but it looks like it's called directly rather than using a yield. What am I missing here?

It's not async — the `return {Promise<Object>}` comment is just outdated. `item.getField()` isn't async — item data is loaded either for a whole library at a time (automatically when a library is viewed for the first time in the UI) or when Zotero.Items.getAsync() is used (for rare cases where an item might be accessed before that happens). Either way, once there's a Zotero.Item, all item data is available.

(It's easy enough to confirm this — if the return value doesn't have a .then, it's not a promise.)

I had forgotten -- only standalone, so my current testsuite (which needs the FF plugin to test) won't work. Can I unpack the Linux release (or beta as of yet) version in some way, chuck all the existing tests, plug in my own, and expect to be able to run that on Circle/Travis? I'd rather not test against current-master versions of Zotero. 

[Dan Stillman]

On 2/22/17 5:51 PM, Emiliano Heyns wrote:
> I had forgotten -- only standalone, so my current testsuite (which
> needs the FF plugin to test) won't work. Can I unpack the Linux
> release (or beta as of yet) version in some way, chuck all the
> existing tests, plug in my own, and expect to be able to run that on
> Circle/Travis? I'd rather not test against current-master versions of
> Zotero.

I'm not sure I understand the question, but we're still running tests
against Firefox Unbranded, and have no immediate plans to change that. I
suspect we'll just do it this way until Electron.

[Emiliano Heyns]

My test suite works by using selenium to install zotero and bbt in Firefox and then have that selenium run my cucumber tests (written in ruby) against that. If there's still a plugin version of zotero that'd be easiest for me to test against,but I couldn't find it, and I don't want to test against latest-from-master zotero but against the version of zotero that the user will have installed. 

[Dan Stillman]

On 2/23/17 1:20 AM, Emiliano Heyns wrote:
> My test suite works by using selenium to install zotero and bbt in
> Firefox and then have that selenium run my cucumber tests (written in
> ruby) against that. If there's still a plugin version of zotero that'd
> be easiest for me to test against,but I couldn't find it, and I don't
> want to test against latest-from-master zotero but against the version
> of zotero that the user will have installed.

We won't be releasing XPIs anymore, but you can still use the 5.0 XPI
build script, build_xpi, with an arbitrary tag (which we'll still be
setting on releases):

https://github.com/zotero/zotero-build/tree/master/xpi

(The standalone build scripts use an XPI as input, so the code will be
the same.)

[Emiliano Heyns]

Could "latest" be added as an alias for the latest tag in build_xpi?

[Emiliano Heyns]

When running "build_xpi", I end up with

FileNotFoundError: [Errno 2] No such file or directory: '/Users/emile/zotero/zotero-build/xpi/zotero-transfw/framework.js'

[Dan Stillman]

On 2/23/17 2:59 AM, Emiliano Heyns wrote:
> Could "latest" be added as an alias for the latest tag in build_xpi?

We have no need for that in the build script, and "latest" wouldn't
always be sufficient with multiple branches (there could be interspersed
tags from multiple branches, there could be alpha/beta tags, etc.), but
you can certainly write code to calculate the tag you want and pass it
to the script.

[Dan Stillman]

On 2/23/17 3:08 AM, Emiliano Heyns wrote:
> When running "build_xpi", I end up with
>
> FileNotFoundError: [Errno 2] No such file or directory:
> '/Users/emile/zotero/zotero-build/xpi/zotero-transfw/framework.js'

You likely didn't do a recursive checkout.

[Emiliano Heyns]

The response to "-h" needs to be updated I think, the 5.0 branch no longer exists.

[Dan Stillman]

On 2/23/17 3:30 AM, Emiliano Heyns wrote:
> The response to "-h" needs to be updated I think, the 5.0 branch no
> longer exists.

No, it doesn't exist yet. This build script only works with Zotero 5.0
or later, but 5.0 is still on 'master' (and we'll probably try to keep
it there for a while). In any case, those are just examples, so you'll
have to adjust as necessary.

[Emiliano Heyns]

ISTR something about Zotero not being too happy about having many tags in its database in 4.0. Is this still the case for 5.0, or has the promisification alleviated this?

[Dan Stillman]

Various operations involving tags have been sped up, though there's
still more optimization work to do:

https://forums.zotero.org/discussion/64616/cant-delete-a-tag

[Emiliano Heyns]

How about a significant number of tags but each assigned to only one or two references? I'd love to get rid of saving the citekey in the extra field, and I've been toying with having it as a tag instead. I see there are several types of tags, what distinguishes these types? 

[Dan Stillman]

On 3/2/17 3:30 PM, Emiliano Heyns wrote:
> How about a significant number of tags but each assigned to only one
> or two references? I'd love to get rid of saving the citekey in the
> extra field, and I've been toying with having it as a tag instead.

The Extra field is really the right place for this — that's what
everyone is using for pseudo-fields that will eventually make their way
into the official data model.

> I see there are several types of tags, what distinguishes these types?

Current tag types are 0 (manual) and 1 (automatic). There may be other
types in the future (e.g., private/workflow tags), but 0 and 1 are the
only types that are currently allowed and the only ones that can sync.

[Emiliano Heyns]

On Thursday, March 2, 2017 at 9:36:24 PM UTC+1, Dan Stillman wrote:

On 3/2/17 3:30 PM, Emiliano Heyns wrote:
> How about a significant number of tags but each assigned to only one
> or two references? I'd love to get rid of saving the citekey in the
> extra field, and I've been toying with having it as a tag instead.

The Extra field is really the right place for this — that's what
everyone is using for pseudo-fields that will eventually make their way
into the official data model.

I'm one of those everyones, but I was hoping to get away from it.

Is there a way to save an item that doesn't trigger observers (or at least excludes specific ones)? I react to changes to items, some of these changes may cause the citation key to change, which I'd then save in the extra field, which cause triggers the notifiers, etc. Right now I check whether the citekey generated after the notification is unchanged and don't save the item in that case, but if there's a way to not have to do that, that'd be great. I'm in the process of cleaning up BBT so as to minimize the porting work.

> I see there are several types of tags, what distinguishes these types?

Current tag types are 0 (manual) and 1 (automatic). There may be other
types in the future (e.g., private/workflow tags), but 0 and 1 are the
only types that are currently allowed and the only ones that can sync.

Ah. OK. 

[Dan Stillman]

On 3/2/17 3:44 PM, Emiliano Heyns wrote:

On Thursday, March 2, 2017 at 9:36:24 PM UTC+1, Dan Stillman wrote:

On 3/2/17 3:30 PM, Emiliano Heyns wrote:
> How about a significant number of tags but each assigned to only one
> or two references? I'd love to get rid of saving the citekey in the
> extra field, and I've been toying with having it as a tag instead.

The Extra field is really the right place for this — that's what
everyone is using for pseudo-fields that will eventually make their way
into the official data model.

I'm one of those everyones, but I was hoping to get away from it.

I don't see why — it's by far the simplest solution until we have a better one.

Is there a way to save an item that doesn't trigger observers (or at least excludes specific ones)? I react to changes to items, some of these changes may cause the citation key to change, which I'd then save in the extra field, which cause triggers the notifiers, etc. Right now I check whether the citekey generated after the notification is unchanged and don't save the item in that case, but if there's a way to not have to do that, that'd be great. I'm in the process of cleaning up BBT so as to minimize the porting work.

You can skip various actions [1], but if you modify visible, syncing data then most of those need to trigger. I'm not totally clear what you're trying to avoid, though.

[1] https://github.com/zotero/zotero/blob/7ccf781add1eedcb57c3b4073672e6d9cb8d1224/chrome/content/zotero/xpcom/data/dataObject.js#L816

[Emiliano Heyns]

On Thursday, March 2, 2017 at 10:04:16 PM UTC+1, Dan Stillman wrote:

The Extra field is really the right place for this — that's what
everyone is using for pseudo-fields that will eventually make their way
into the official data model.

I'm one of those everyones, but I was hoping to get away from it.

I don't see why — it's by far the simplest solution until we have a better one.

Aesthetics for one, better editability for the end user for another, but also as an attempt to get away from a save-cycle as I tried to describe.

Is there a way to save an item that doesn't trigger observers (or at least excludes specific ones)? I react to changes to items, some of these changes may cause the citation key to change, which I'd then save in the extra field, which cause triggers the notifiers, etc. Right now I check whether the citekey generated after the notification is unchanged and don't save the item in that case, but if there's a way to not have to do that, that'd be great. I'm in the process of cleaning up BBT so as to minimize the porting work.

You can skip various actions [1], but if you modify visible, syncing data then most of those need to trigger.

I'm modifying data that I want to have synced (the "extra" field for the moment until there is an alternative). It is visible there, but that's a side-effect rather than a desideratum for cite keys from my point of view. Would "skipNotifier" be harmful to this use? 

  

I'm not totally clear what you're trying to avoid, though.

I'm trying to prevent a save-loop.

[Dan Stillman]

On 3/2/17 4:21 PM, Emiliano Heyns wrote:

 

I'm not totally clear what you're trying to avoid, though.

I'm trying to prevent a save-loop.

I don't see how a save loop and skipping notifier events are related. You have to watch for notifier events that happen after normal saves in order to know if you need to update the citation key. You then need to save the item again to update the citation key. So that's two saves regardless. The extra notifier events that happen from your save just allow the rest of Zotero to react to your changes. You shouldn't skip those.

You could obviously monkey-patch things to set the citation key during the initial save (not that I'd recommend that), but that wouldn't have anything to do with the notifier events.

We could possibly add a pre-save hook to data objects that allowed changes to be made to the data being saved, but we'd have to carefully consider the implications of that.

[Emiliano Heyns]

On Thu, Mar 2, 2017 at 10:31 PM, Dan Stillman <dsti...@zotero.org> wrote:

I don't see how a save loop and skipping notifier events are related. You have to watch for notifier events that happen after normal saves in order to know if you need to update the citation key. You then need to save the item again to update the citation key. So that's two saves regardless.

I'm not worried about those two, my worry is about the notification from the second, which will cause me to re-calculate the citation key, and to re-save it, and...

I currently do recalculate and just don't save when it's unchanged, but I'm actively looking for places where I can drop code, and certainly where it involves touching the DB.

 

The extra notifier events that happen from your save just allow the rest of Zotero to react to your changes. You shouldn't skip those.

OK.

 

You could obviously monkey-patch things to set the citation key during the initial save (not that I'd recommend that), but that wouldn't have anything to do with the notifier events.

I'm also looking to scratch as many monkey-patches as I can.

 

We could possibly add a pre-save hook to data objects that allowed changes to be made to the data being saved, but we'd have to carefully consider the implications of that.

That would be very helpful indeed. The citation key is in many cases a derived property.

 

[Dan Stillman]

On 3/2/17 5:01 PM, Emiliano Heyns wrote:

On Thu, Mar 2, 2017 at 10:31 PM, Dan Stillman <dsti...@zotero.org> wrote:

I don't see how a save loop and skipping notifier events are related. You have to watch for notifier events that happen after normal saves in order to know if you need to update the citation key. You then need to save the item again to update the citation key. So that's two saves regardless.

I'm not worried about those two, my worry is about the notification from the second, which will cause me to re-calculate the citation key, and to re-save it, and...

I currently do recalculate and just don't save when it's unchanged, but I'm actively looking for places where I can drop code, and certainly where it involves touching the DB.

I don't see why there'd be any extra code here. The same check that runs the first time would run the second, but the second time there wouldn't be any changes. Also not clear on "touching the DB" — do you need to access the DB to generate a citation key?

But I recently did add a way to pass along arbitrary notifier data with a save:

yield item.saveTx({
    notifierData: {
        bbtCitekeyUpdate: true
    }
)

Then in the notify(event, type, ids, extraData), check for extraData[id].bbtCiteKeyUpdate and ignore if present.

(This hasn't been tested extensively, so let me know if you run into trouble.)

Also, I forget the specifics, but there should be info in extraData[id] for a 'modify' that tells you which fields changed, which hopefully you're using to avoid running the regeneration unless relevant fields have changed. (This probably also needs more test coverage.)

We could possibly add a pre-save hook to data objects that allowed changes to be made to the data being saved, but we'd have to carefully consider the implications of that.

That would be very helpful indeed. The citation key is in many cases a derived property.

I can imagine something like this:

var updateCitationKey = function (env, obj) {
    obj.setField('extra', generateExtra(obj));
};

Zotero.Items.onBeforeSave.addListener(updateCitationKey);

(Or, ideally, some documented visibility into the internal obj._changed properties so you could see what fields changed without regenerating the whole thing.)

One problem with this is that, if multiple observers were registered, the order would be undefined, and you couldn't really guarantee that you were seeing the final data being saved. So in that way just using notifier observers after the save makes more sense.

[Emiliano Heyns]

On Thu, Mar 2, 2017 at 11:21 PM, Dan Stillman <dsti...@zotero.org> wrote:

 do you need to access the DB to generate a citation key?

Indirectly -- I need to access (potentially) all the reference fields.

 

But I recently did add a way to pass along arbitrary notifier data with a save:

yield item.saveTx({
    notifierData: {
        bbtCitekeyUpdate: true
    }
)

Then in the notify(event, type, ids, extraData), check for extraData[id].bbtCiteKeyUpdate and ignore if present.

(This hasn't been tested extensively, so let me know if you run into trouble.)

That's fantastic, that solves the issue for me. This is 5.0-only then?

 

Also, I forget the specifics, but there should be info in extraData[id] for a 'modify' that tells you which fields changed, which hopefully you're using to avoid running the regeneration unless relevant fields have changed. (This probably also needs more test coverage.)

That's harder to use effectively since my key generator uses a fair chunk of the fields. Although not the extra field.

 I can imagine something like this:

var updateCitationKey = function (env, obj) {
    obj.setField('extra', generateExtra(obj));
};

Zotero.Items.onBeforeSave.addListener(updateCitationKey);

(Or, ideally, some documented visibility into the internal obj._changed properties so you could see what fields changed without regenerating the whole thing.)

One problem with this is that, if multiple observers were registered, the order would be undefined, and you couldn't really guarantee that you were seeing the final data being saved. So in that way just using notifier observers after the save makes more sense.

Fair enough. The earlier mechanism your described is sufficient for my needs.

[Emiliano Heyns]

On Thursday, March 2, 2017 at 11:21:32 PM UTC+1, Dan Stillman wrote:

Also, I forget the specifics, but there should be info in extraData[id] for a 'modify' that tells you which fields changed, which hopefully you're using to avoid running the regeneration unless relevant fields have changed. (This probably also needs more test coverage.)

So this means if I set up a notifier like so:

Zotero.Notifier.registerObserver(this, ['item'])

and that notifier is passed in

action :"modify", type :"item",  ids :[2],  extraData":{"2":{}}

 

I can safely assume no fields were changed? That would be relevant info for me. In this case, an attachment was added.

[Emiliano Heyns]

When I modify an item (setting the "extra" field to "updated" where it was previously empty), I still get

{"action":"modify","type":"item","ids":[2],"extraData":{"2":{}}}

 I don't see anything relating to the field update. I've found this discussing the feature here and that says (I think) there's supposed to be an "extraData["2"].changes", should that still be the case?

[Dan Stillman]

Yeah, it looks like this isn't working at the moment.

What exactly do you need to know from this notification? Do you need the previous state of the entire item, the previous values of the changed fields, or just the list of fields that changed? This previously included the full pre-change state, but that might be excessive. It's possible this should just include the list of changed fields.

[Emiliano Heyns]

For BBT, just knowing whether any fields have changed suffices. If no fields have changed, like when an attachment is added, I don't need to bother regenerating the citation key. 

[Emiliano Heyns]

Correction: a list of fields would be preferable. Should I stay with sticking the cite key in the extra field vs putting it in an attachment, I don't want to regenerate when the extra field changes. 

[Emiliano Heyns]

On Sat, Feb 18, 2017 at 9:36 PM, Dan Stillman <dsti...@zotero.org> wrote:

On 2/18/17 3:26 PM, Emiliano Heyns wrote:

On Wednesday, October 5, 2016 at 11:25:54 AM UTC+2, Emiliano Heyns wrote:

Can you explain how Zotero.Schema.schemaUpdatePromise works, technically? It looks like it enables any number of places to wait on the promise. I could use something like this to do dependent loads in BBT, e.g. I'd want parts of the BBT code to only proceed after I've loaded the BBT database.

What is the state of the database after schemaUpdatePromise resolves? I know some things are in memory after startup (like collections) but are (parts of) the references also in memory after this resolves?

Collections and searches from all libraries should be loaded, and items from the currently selected library should be loaded. Items from other libraries won't be loaded. (Specifically, in Standalone, schema updating waits for Zotero.uiReadyPromise, which is resolved once items for the current library are loaded.)

If you need items that might not be loaded, you can use Zotero.Items.getAsync() (or getByLibraryAndKeyAsync) to get them. When you know that they're loaded, the synchronous versions are better.

Better in what way? And how can I tell whether they're loaded?

[Dan Stillman]

Better in that synchronous calls are faster.

You don't need to test for whether the items are loaded — you just either use get() or getAsync() depending on whether the objects might be unloaded, based on what I said above. So, e.g., in zotero:// protocol handler stuff we use getAsync(), because a zotero:// URL could be loaded externally for an item in a library hasn't been opened yet in the Zotero UI.

[Emiliano Heyns]

On Sat, Aug 5, 2017 at 10:03 PM, Dan Stillman <dsti...@zotero.org> wrote:

Better in what way? And how can I tell whether they're loaded?

Better in that synchronous calls are faster.

You don't need to test for whether the items are loaded — you just either use get() or getAsync() depending on whether the objects might be unloaded, based on what I said above. So, e.g., in zotero:// protocol handler stuff we use getAsync(), because a zotero:// URL could be loaded externally for an item in a library hasn't been opened yet in the Zotero UI.

But because parts of my code (like auto-export) by stuff that may not have been opened in the UI, I'm better off sticking with getAsync, it seems.