Closed Bug 982115 Opened 7 years ago Closed 6 years ago

Async Places Transactions: Solution for implementing Cancel/Undo in bookmarks dialog and Star UI

Categories

(Toolkit :: Places, defect)

x86
macOS
defect
Not set
normal
Points:
8

Tracking

()

RESOLVED FIXED
mozilla36
Iteration:
36.2

People

(Reporter: mano, Assigned: mano)

References

Details

Attachments

(1 file)

The new transactions manager has no proper "batching" concept like nsITransactionManager's begin/endBatch.  However, |transact| accepts a generator function for its input (the consumers "yields" transactions and when the generator returns there's only a single entry in the transaction history). This mode resembles old-style batching, but it's mostly intended for replacing "child transaction" usage (e.g. "Bookmark All Tabs").

In the past, consumers of nsITransactionManager had trouble with transaction-less batches. nsITransactionManager used to consider those batches a no-op, so that if undo is called after such a batch was closed, the "previous" undo entry, if any, would be undone. For places, the workaround was to commit a dummy transaction whenever beginBatch was called. For the HTML5 Undo Manager, an "allowEmpty" argument was added to endBatch.

Moving forward with the new places-oriented API, I think the generator-mode isn't going to work for "batching" in the edit-bookmark star-popup (I think we'll add something like undoUntil for that). Since that's the only case in which the allowEmpty/dummy-transaction solution made sense, the initial implementation of |transact| works "the old way" - i.e. if no transaction is yielded, the transaction history is left unchanged. We may want to revise this decision as consumers are converted to the new API.
The only problematic case is the star panel, and I don't think we can use transact there anyway, thus I think the current semantics are fine. Therefore I'm morphing this bug to cover a proper solution for the star panel issue (which cannot batch by transact). After-the-fact transactions grouping is one option.
Summary: New transactions manager: Figure out if |transact| should force an entry in the transaction history if no transactions are comited → Async Places Transactions: Solution for implementing Cancel/Undo in the star panel
Assignee: nobody → mano
Status: NEW → ASSIGNED
Attachment #8515898 - Flags: review?(mak77)
Marco, please add this to the current iteration. It blocks bug 951651.
Iteration: --- → 36.2
Points: --- → 8
Summary: Async Places Transactions: Solution for implementing Cancel/Undo in the star panel → Async Places Transactions: Solution for implementing Cancel/Undo in bookmarks dialog and Star UI
Note that while I'm happy with these API changes regardless of the reason they're necessary now (I was never satisfied with my custom Task.spawn trick), I don't consider this a good solution for the Cancel/Undo use case at all; So I filed bug 1093030 for figuring out something else. However, for the time being this will do. I'll just need to be careful in bug 951651 not to end up with any nested batches.
Comment on attachment 8515898 [details] [diff] [review]
patch (see included commit message for the details)

Review of attachment 8515898 [details] [diff] [review]:
-----------------------------------------------------------------

some things to fix, but nothing blocking.

::: browser/components/places/PlacesUIUtils.jsm
@@ +398,5 @@
>     * @param aCopy
>     *        The drag action was copy, so don't move folders or links.
>     *
> +   * @returns a Places Transaction that can be transacted for performing the
> +   * move/insert command.

@return (no s) and please indent it as the params

::: toolkit/components/places/PlacesTransactions.jsm
@@ +21,4 @@
>   *
> + * PTM shares most of its semantics with common command pattern implementations.
> + * However, the asynchronous design of contemporary and future Places APIs (e.g.
> + * Bookmarks.jsm), combined with the commitment to serialize all UI operations,

s/Places// and I'd also remove the example...
Otherwise this comment may not mean a thing in a year from now :)

@@ +60,5 @@
>   * To make things simple, a given input property has the same basic meaning and
>   * valid values across all transactions which accept it in the input object.
>   * Here is a list of all supported input properties along with their expected
>   * values:
> + *  - url: a URL object, an nsIURI object, or a url spec string.

fwiw, I think we may use the term "href" rather than "url spec" around.

@@ +62,5 @@
>   * Here is a list of all supported input properties along with their expected
>   * values:
> + *  - url: a URL object, an nsIURI object, or a url spec string.
> + *  - urls: an array of urls, as above.
> + *  - feedURL: an URL (as above), holding the url for a live bookmark.

I supposed "array of urls" (lowercase) was cause each can be either URL, nsIURI or href. But now I'm not sure if this uppercare URL means a different thing.
I think it's ok if you use uppercase URL in any case, provided we are consistently using uppercase or lowercase everywhere (apart from property names) to not confuse the reader

@@ +132,5 @@
> + * batch is still running, the new batch is enqueued with all other PTM work
> + * and thus not run until the running batch ends. The same goes for undo, redo
> + * and clearTransactionsHistory (note batches cannot be done partially, meaning
> + * undo and redo calls that during a batch are just enqueued).
> + * IT"S PARTICULARLY IMPORTANT NOT TO YIEDD ANY PROMISE RETURNED BY ANY OF THESE

typo YEDD

I'd also add a newline above this to make it even more emphasized

@@ +133,5 @@
> + * and thus not run until the running batch ends. The same goes for undo, redo
> + * and clearTransactionsHistory (note batches cannot be done partially, meaning
> + * undo and redo calls that during a batch are just enqueued).
> + * IT"S PARTICULARLY IMPORTANT NOT TO YIEDD ANY PROMISE RETURNED BY ANY OF THESE
> + * METHODS FROM A BATCH FUNCTION. UNTIL WE FIND A WAY TO THROW IN THAT CASE (SEE

I find a little bit unclear what are "ANY OF THESE METHODS", I think it could be better clarified.

@@ +306,3 @@
>             o => !TransactionsHistory.isProxifiedTransactionObject(o))) {
>          throw new Error("aToTransact contains non-transaction element");
>        }

should we throw for aToBatch.length == 0? is there any case where that is a valid input?

@@ +307,5 @@
>          throw new Error("aToTransact contains non-transaction element");
>        }
> +      return TransactionsManager.batch(function* () {
> +        for (let txn of aToBatch)
> +          yield txn.transact();

what happens when any of these txn fails?
Do we undo all of the transactions executed so far, or continue reporting the error?

This doesn't seem to handle any of those.

@@ +400,5 @@
>    get topRedoEntry() TransactionsHistory.topRedoEntry
>  };
>  
>  /**
> + * Helper for serializing Task.spawn calls.

worth expading this a little bit more (when is it used, who should use it and how)

@@ +413,5 @@
> +   *
> +   * @param aFunc
> +   *        @see Task.spawn.
> +   * @return a promise that resolves once aFunc is done running. The promise
> +   * "mirrors" the aFunc's promise.

please indent return as param

@@ +416,5 @@
> +   * @return a promise that resolves once aFunc is done running. The promise
> +   * "mirrors" the aFunc's promise.
> +   */
> +  enqueue(aFunc) {
> +    let promise = this._promise.then(() => Task.spawn(aFunc));

would .then(Task.async(aFunc)) work here?

@@ +424,5 @@
> +    return promise;
> +  },
> +
> +  /**
> +   * Enqueue a promise.

commetn about how this differs from enqueue and when it's supposed to be used?

@@ +430,5 @@
> +   * @param aPromise
> +   *        any promise.
> +   */
> +  alsoWaitFor(aPromise) {
> +    this._promise = Promise.all([this._promise.catch(() => {}), aPromise]);

could you please add a comment about the purpose of this empty catch?

@@ +447,5 @@
> +  _mainEnqueuer: new Enqueuer(),
> +  _transactEnqueuer: new Enqueuer(),
> +
> +  _batching: false,
> +  _createdBatchEntry: false,

please document what these 2 properties are tracking

@@ +827,5 @@
>  };
>  
>  // Update the documentation at the top of this module if you add or
>  // remove properties.
> +DefineTransaction.defineInputProps(["url", "feedURL", "siteURL"],

nit: since we do somethingGuid, I wonder if here we should do somethingUrl.

::: toolkit/components/places/tests/unit/test_async_transactions.js
@@ +458,3 @@
>      let bkmTxn = PT.NewBookmark(bm_info);
> +    bm_info.guid = yield bkmTxn.transact();
> +    return { folderTxn, bkmTxn };

hm, this looks fishy... did you mean to return an array?

@@ +948,5 @@
>    ensureUndoState();
>  });
>  
>  add_task(function* test_edit_url() {
> +  return;

this is skipping the test... if done on purpose, it needs a bug filed and a todo.
Attachment #8515898 - Flags: review?(mak77) → review+
> ::: toolkit/components/places/tests/unit/test_async_transactions.js
> @@ +458,3 @@
> >      let bkmTxn = PT.NewBookmark(bm_info);
> > +    bm_info.guid = yield bkmTxn.transact();
> > +    return { folderTxn, bkmTxn };
> 
> hm, this looks fishy... did you mean to return an array?

Nope :)
https://hg.mozilla.org/mozilla-central/rev/d265e96ff557
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Flags: firefox-backlog+
You need to log in before you can comment on or make changes to this bug.