Improve handling of conflicts due to concurrent ticket edits

[anonymous]

It should be possible to prevent conflicts and loss of data during concurrent editing of a ticket by multiple users.

Related ticket:152 "Improve handling of conflicts due to concurrent Wiki edits"

Multiple concurrent edits to tickets occur in our work environment frequently (even among a team of only 7 people). Our site uses 0.10.4 — I don't know if it will have been addressed by #152 in version 0.11, but I thought it deserved special mention in case ticket handling was different enough from general wiki page editting.

[Christian Boos]

0.11 is already a big improvement over 0.10 w.r.t. concurrent edits. Please try it out (TracDownload) and tell us what you think.

Now, in 0.11, the behavior is slightly different between wiki and ticket. In the solution used for wiki, we show a diff between the version you were about to save and the newer one someone managed to save before.

When there's a conflict for ticket editing, we also inform the user about the conflict and show what would be different from the newer ticket version, but:

• we don't distinguish between the changes made by the other that we're simply reverting from those we're in conflict with, or the changes that "apply cleanly"
• we don't show a diff for description or other text fields changes
• we don't offer a way for the user to simply update her modifications and make the change happen; instead she has to save her comment (copy/paste), reload the ticket, redo the changes

So there's obviously still some room from improvement (as mentioned in the original #4100 ticket).

[gkovacs@…]

Replying to cboos:

• we don't offer a way for the user to simply update her modifications and make the change happen; instead she has to save her comment (copy/paste), reload the ticket, redo the changes

For our team rejected new comments are still the most annoying. What about accepting a new comment after having a preview that shows the already submitted (concurrent) comments? Preview would display a big warning message about ticket change (as it does already) and the need to review/modify the new comment. Making a submit from the preview page the new comment would be accepted. (Technically, this means only that preview would update the page's time stamp, or not?) Trying to submit new comments without preview would redirect to preview in this case. In case of changes in other fields the current rejection behavior could be followed.

[Christian Boos]

Milestone 1.0 deleted

[Remy Blank]

I also find this issue very annoying. Now that we have automatic preview of ticket comments, we could also make new comments appear "live" with a hint that they are new (background color?), and possibly show conflicts in ticket fields.

I'll try a few experiments.

[Mitar]

Just possibility to submit changes after the ticket has been reloaded with new changes and a warning displayed would be enough. So there is a conflict, you get ticket form back, with changes from before colored in some color, you check them, update (or not) your change based on that and you submit. Why it this last step prevented? What is different with opening a new form and copying changes there and submit which is possible now? Why the system cannot do this by itself?

[Remy Blank]

Improved ticket change preview.

[Remy Blank]

7145-ticket-change-preview-r10500.patch​ is a "technology preview" for an improved automatic preview functionality that allows a full form to be previewed when some of its fields change. The new functionality is shown for ticket changes, which are now previewed including the field changes. The implementation is still a bit rough, but shows where I would like to go with automatic previews. Feedback is very welcome.

As a next step, validation warnings as well as mid-air collisions could be transmitted with the preview and displayed above it.

[Remy Blank]

Fully dynamic ticket creation and edition.

[Remy Blank]

7145-ticket-preview-r10560.patch​ implements a fully dynamic ticket creation and editing using the auto-submit feature. The effect is pretty nice, if I may say so myself :)

It would be great to get some feedback for this feature before I move on to the real subject, i.e. handling conflicts. The idea would be to transmit and display the collision warning through the same channel, possibly with the added comments, and to provide a way to submit anyway.

[Mitar]

Patch is against current trunk? Or it is from some branch on github/bitbucket?

[Remy Blank]

The patch is against trunk at [10560].

[Christian Boos]

I've tested it a bit, and it seems to work really well. The code is also fine by me.

The only reservation I have is about the placement of the comment preview: now it's a the bottom of the page, so quite far away the comment textarea itself, if you have the ticket modify panel opened (and if you have lots of fields and even wiki fields like we have here, that's even quite far). If it was kept just below the textarea, it would also be close to the ticket modify form.

Besides, I wonder if in a second step there wouldn't be even better way to do the relative placement of the forms and the previews. Maybe a side-by-side mode like on the Wiki pages? The fixed width for the #content would have to be disabled in this mode, of course. Maybe tabs? The preview would be differed to the moment you switch the tabs. Well, I will sleep over that ;-)

[Remy Blank]

Replying to cboos:

I've tested it a bit, and it seems to work really well. The code is also fine by me.

Thanks for testing!

The only reservation I have is about the placement of the comment preview: now it's a the bottom of the page, so quite far away the comment textarea itself, if you have the ticket modify panel opened (and if you have lots of fields and even wiki fields like we have here, that's even quite far). If it was kept just below the textarea, it would also be close to the ticket modify form.

Yes, maybe it was better right below the comment editor. Except for the fact that the property editors jump up and down a bit when changing properties, due to the height of the preview changing. My thinking was that the comment text would be edited with the property editor panel closed, so it wouldn't make such a big difference.

There's also the issue of the ticket box. When creating a new ticket, I put it at the bottom, so that the editors don't get pushed around when it updates. When editing an existing ticket, they do get pushed around, and that's not really optimal.

Besides, I wonder if in a second step there wouldn't be even better way to do the relative placement of the forms and the previews. Maybe a side-by-side mode like on the Wiki pages?

I was thinking exactly that while reading your comment. We should have enough horizontal space, and side-by-side editing works wonderfully for the wiki. Moreover, this would allow displaying validation and "mid-air collision" warning dynamically at the top.

Putting the ticket property editors next to the ticket box absolutely makes sense. What about the comment editor? Having it placed right below the last comment was ideal, as one often replies to one of the last comments. Placing it below the property editors in the second column would often prevent seeing the interesting comments. And placing it in the second column next to the last comment would separate both editors, which isn't too nice either.

I'll experiment with that a bit. But first I'll move the comment box back below the comment editor.

Maybe tabs? The preview would be differed to the moment you switch the tabs.

I don't like the idea of having to press a tab to view the preview, as it's basically not better than pressing the "Preview" button :)

[Remy Blank]

Same patch, but ticket comment preview is right below the comment editor.

[Remy Blank]

7145-ticket-preview-2-r10560.patch​ moves the comment preview back below the comment editor. I have a small preference for the comment below the property editor (i.e. the previous patch), because:

• It doesn't make a big difference when the "Modify ticket" heading is closed, which is the starting state when I add a comment (I usually write the comment, then edit fields and submit).
• The ticket property editor doesn't jump around when modifying properties.

[Remy Blank]

Comment preview back on top of properties, but property section collapses on preview if the focus is in the comment editor.

[Remy Blank]

In 7145-ticket-preview-r10632.patch​, the comment preview is again back on top of the property editor. On previewing a comment, the property editor is collapsed if the focus is in the comment editor. This makes the comment preview visible if the property editor is large. Nice idea, Christian!

The patch was also refreshed for current trunk.

[Remy Blank]

Applied 7145-ticket-preview-r10632.patch​ in [10634].

The next step is to include information about ticket changes that have occurred since an edit has started in the automatic preview, to display them to the user with an adequate warning, and to allow the user to adapt and apply her changes.

[Remy Blank]

Improve handling of ticket comment collisions.

[Remy Blank]

7145-concurrent-editing-r10647.patch​ is a first shot at improving the handling of collisions when editing tickets. It works as follows:

• Auto-preview adds new ticket comments to the change log.
• Two timestamps are tracked: the edit start time, and the last change time of the ticket at the time when new changes have been shown. The former is set when arriving at the ticket page, and isn't changed afterwards. The latter is updated when clicking "Preview", or when new comments are added during auto-preview.
• Changes added since edit start time are marked with a red shade (I don't really like the current color, so any better shades welcome).
• Submitting fails if the last change time of the ticket is different from the ticket change time when new changes were last transmitted (due to auto-preview or the "Preview" button).

So the normal "conflict" workflow with a browser supporting JavaScript is now the following:

• The user edits the comment and ticket fields.
• Any new comments are displayed on auto-preview (along with a warning).
• The user can adapt her changes to the incoming modifications.
• Finally, the user can click "Submit changes" and the change will be accepted.
• If a new change happened to be done between the last update due to auto-preview and the user clicking "Submit changes", the validation will fail and the ticket will return to preview mode, allowing the user to view the new change and possibly adapt her changes.

The workflow without JavaScript is very similar, except that it requires explicitly hitting the "Preview" button.

The patch also moves cnum handling into the Ticket class instead of using an explicit cnum field in the ticket form. This should fix at least some of the issues we have been seeing with comment numbering (not the most recent one, though).

Review, testing and improvement suggestions very welcome.

[Christian Boos]

Looks promising… but doesn't work very well yet in my testing (chrome 10.0.648.151 beta on Windows):

• after ticket creation, the This ticket has been modified since you started editing… notification is also shown
• same thing when viewing a newly created ticket … correction: same thing when viewing any existing ticket, and after any modification: well, it's always there ;-)
• saving a change currently needs two submits if an automatic preview: the first submit show the changes as a preview, with the notice, the second show the change as effective (but still with the notice, as always)

I haven't investigated yet why this fails, it's probably a simple thing.

Other suggestions:

• The modifications are highlighted above in red. → The <span class="trac-new">other modifications</span> are highlighted.
• we could use the same highlighting than what we have in the timeline, to suggest "something new happened in the meantime", i.e:

  border-left: 0.31em solid #C0F0C0;
  padding-left: 0.31em;
  

[Christian Boos]

example of timeline's style left green border

[Christian Boos]

A few follow-up testing: same behavior with FF 4.0, so it's not an issue with Chrome. Also, the only time the Change notification is not shown is after an automatic preview (it's there with an explicit "Preview").

[Remy Blank]

That's weird, because I did all my testing with Firefox (but 3.6, so it may be version-dependent). In particular, I have only needed a double-submit if a concurrent modification hadn't been shown (as it should). I hadn't tested ticket creation, so this may indeed not work correctly.

Good ideas with the green bar and the wording, will fix.

[Christian Boos]

view_time is never set, so start_time == view_time is always False.

[Remy Blank]

Replying to cboos:

view_time is never set, so start_time == view_time is always False.

Weeell… It should :)

Oh, shoot, last-minute refactoring gone bad. Replace view_time with ticket['changetime'] in the expression above.

[Remy Blank]

Mmh, that's not enough. It seems that ticket['changetime'] isn't interpreted correctly in the template. Let me see…

[Remy Blank]

Ok, same change in ticket_preview.html, replace view_time with ticket['changetime'].

New patch with suggestions from comment:19 coming tonight. Sorry for the confusion.

[Remy Blank]

Improved concurrent editing.

[Remy Blank]

7145-concurrent-editing-2-r10647.patch​ should work better, and includes the suggestions from comment:19. It also un-threads the changelog before updating. The strange way this is done (un-checking, then calling click(), and un-checking again) is the only reliable way that I have found to trigger the right code path for un-threading and having the check box un-checked afterwards. Better ideas welcome.

The green bar for new comments looks great!

[Christian Boos]

Right, I believe that now it works as you intended it to work ;-)

There's a glitch when the conflict happen for a newly created ticket, more specifically when you're writing the first comment and get beaten to it: you get the notification but the other modifications are not visible since in this situation we don't have a #changelog yet. Maybe we should simply toggle the visibility of this whole block instead of the more radical <py:if>.

About the trac-threaded-toggle problem, I believe this is because we test for the .checked property to determine the state of the toggle, and that property is certainly only set after our handler is called. Modifying the handler to use a toggleClass instead makes the code simpler:

trac/htdocs/js/threaded_comments.js

@@ -2 +2 @@
   var comments = null;
   var toggle = $('#trac-threaded-toggle');
   toggle.click(function() {
-    if ($(this).checked()) {
+    $(this).toggleClass('checked');
+    if ($(this).hasClass('checked')) {
       comments = $("div.change");
       comments.each(function() {
         var children = $("a.follow-up", this).map(function() {

trac/ticket/templates/ticket.html

@@ -44 +44 @@
           // Unthread and update changelog
           var threaded_toggle = $('#trac-threaded-toggle');
           if (threaded_toggle.checked())
-            threaded_toggle.attr('checked', false).click().attr('checked', false);
+            threaded_toggle.click();
           $("#changelog .trac-new").remove();
           var new_changes = items.filter("#new-changes").children();
           $("#changelog").append(new_changes);

The notification should also say something about the possibility to "submit" despite the intervening modifications, as this is an important change in behavior from previous versions of Trac:

trac/ticket/templates/ticket.html

@@ -44 +44 @@
           // Unthread and update changelog
           var threaded_toggle = $('#trac-threaded-toggle');
           if (threaded_toggle.checked())
-            threaded_toggle.attr('checked', false).click().attr('checked', false);
+            threaded_toggle.click();
           $("#changelog .trac-new").remove();
           var new_changes = items.filter("#new-changes").children();
           $("#changelog").append(new_changes);
@@ -160 +160 @@
           </h2>
           <div id="trac-edit-warning" class="warning system-message"
                style="${'display: none' if start_time == ticket['changetime'] else None}">
-            This ticket has been modified since you started editing. The
-            <span class="trac-new">other modifications</span> are highlighted.
+            This ticket has been modified since you started editing. You should review the
+            <em class="trac-new">other modifications</em> which have been appended above.
+            You can nevertheless proceed and submit your changes if you wish so.
           </div>
           <!--! Comment field -->
           <fieldset class="iefix">

In the not logged in case, I think we should put the Author or Reporter inputs right before the Preview / Submit button, IOW move the preview up. I also noticed that the changes in that field are not part of the automatic preview (manual preview takes it).

Finally, concerning the .css, as surprising as it seems, there is a visual difference between .3em and .31em, so I think it's better to use the thicker version (also being slightly thicker makes it visually more distinct from the border-left of a citation). I'd also prefer to have the same border-left effect in the notification rather than the background. The notification already has a background, so "stacking" two colors is not that great. The .css becomes:

trac/htdocs/css/ticket.css

@@ -110 +110 @@
 }
 #trac-comment-editor .wikitoolbar { margin-left: -1px }
 #trac-add-comment :link, #trac-add-comment :visited { color: #b00 }
-#changelog .trac-new { border-left: 0.3em solid #c0f0c0; padding-left: 0.3em; }
+.trac-new { border-left: 0.31em solid #c0f0c0; padding-left: 0.31em; }
 #changelog h3, #ticketchange h3 {
  border-bottom: 1px solid #d7d7d7;
  color: #999;
@@ -138 +138 @@
 form .field div.trac-resizable { width: 100% }
 
 #propertyform { margin-bottom: 2em; }
-#trac-edit-warning .trac-new { background-color: #c0f0c0; }
 #properties { white-space: nowrap; line-height: 160%; padding: .5em }
 #properties table { border-spacing: 0; width: 100%; padding: 0 .5em }
 #properties table th {

(this needs the change from <span> to <em> in the previous diff to look good)

Wrap-up patch upcoming.

[Remy Blank]

Replying to cboos:

Maybe we should simply toggle the visibility of this whole block instead of the more radical <py:if>.

Yes, good catch.

About the trac-threaded-toggle problem, I believe this is because we test for the .checked property to determine the state of the toggle, and that property is certainly only set after our handler is called.

I wondered about the .checked() method, as I haven't found any reference to it in the jQuery documentation. Rather, they suggest using .val('checked') to get the state. I tried that, but the result was the same. Using an explicit class feels like a hack, as it duplicates state that is already available. And how come the "checked" state is set before the call when you click on the control, but after the call when you call .click() explicitly?

The notification should also say something about the possibility to "submit" despite the intervening modifications, as this is an important change in behavior from previous versions of Trac:

Yes, absolutely.

I also noticed that the changes in that field are not part of the automatic preview (manual preview takes it).

I hadn't noticed that, and I wonder why this is. The field is part of the form, so it should be included. I'll investigate.

I'd also prefer to have the same border-left effect in the notification rather than the background. The notification already has a background, so "stacking" two colors is not that great.

I did that initially, but felt it didn't look good: the greed bar between two words looks out of place. But I don't like the double background either.

(this needs the change from <span> to <em> in the previous diff to look good)

Maybe this makes it good enough.

Thanks for the feedback! I would like to have this finalized and applied before trying my last idea. When retrieving new entries, I would like to update the form controls as well, but only the ones that haven't been changed explicitly by the user. For example, if I had changed the version and priority, and a new change modifies the priority and milestone, I would like the milestone control to be updated, but not the priority control.

[Christian Boos]

a few follow-ups to attachment:7145-concurrent-editing-2-r10647.patch​

[Christian Boos]

Replying to rblank:

Replying to cboos:

Maybe we should simply toggle the visibility of this whole block instead of the more radical <py:if>.

Yes, good catch.

Fixed in the patch t7145-follow-up-comment28.diff​.

Forgot to note in the patch log that I also fixed the highlighting problem for the "Add a comment" h2, by moving the id="trac-add-comment" to its parent div.

I wondered about the .checked() method, as I haven't found any reference to it in the jQuery documentation.

It's something from us (cmlenz' time), see trac.js.

Using an explicit class feels like a hack, as it duplicates state that is already available.

Well yes, but that state doesn't change in a way that plays nice with the event handlers.

And how come the "checked" state is set before the call when you click on the control, but after the call when you call .click() explicitly?

From jQuery's ​trigger() doc:

Although .trigger() simulates an event activation, complete with a synthesized event object, it does not perfectly replicate a naturally-occurring event.

I suppose we're hitting such a case, the DOM property .checked must be modified after our handler gets called.

I also noticed that the changes in that field are not part of the automatic preview (manual preview takes it).

I hadn't noticed that, and I wonder why this is. The field is part of the form, so it should be included. I'll investigate.

Yep, this was not addressed in my patch. Note that this works in the autopreview for a new ticket, just not for an already existing ticket.

I would like to have this finalized and applied before trying my last idea. When retrieving new entries, I would like to update the form controls as well, but only the ones that haven't been changed explicitly by the user. For example, if I had changed the version and priority, and a new change modifies the priority and milestone, I would like the milestone control to be updated, but not the priority control.

That would be very welcome, yes. Currently, it's far too easy to undo the changes done by the other modifications.

[Remy Blank]

t7145-follow-up-comment28.diff​ integrated and works well, thanks.

There's one more issue: attachments mess up the whole thing.

• Then a new attachment is the last change on a ticket, it is always marked as a new change.
• When trying to add a comment after an attachment, a conflict is always shown. Moreover, the attachment is appended multiple times as a new change. This is due to the fact that the ticket changetime is taken as the start_time, and adding an attachment doesn't modify the changetime.

I'm not sure what the solution should be. Should adding an attachment to a ticket update the changetime? Or should we use the time of the last comment or attachment as a timestamp? The latter would probably be best, so I'll try and see how this can be done.

[Remy Blank]

Fixed most issues.

[Remy Blank]

7145-concurrent-editing-r10668.patch​ implements the following changes after comment:31:

• The author field for anonymous users is now handled correctly. This generated a surprisingly large change, as we previously only transmitted the change content, but not its heading, so the author would never change in the preview.
• Attachments are now handled correctly, by always transmitting the whole changelog on preview (instead of only the changes since edit start). This also has the advantage that the "in reply to" and "follow-up" links in the change headings are updated correctly. This may sound inefficient, but we were already computing the data for all changes anyway, so it's only worse in the sense that more data is transmitted.
• The ticket (comment) deleter component has been updated to identify comments by their timestamp instead of the comment number. This should ensure that the right comment is deleted, even with duplicated comment numbers like we have seen lately.

Feedback welcome.

[Remy Blank]

Updated patch for current trunk.

[Christian Boos]

I tested the patch, I think it's OK now. The update of the username for anonymous is working fine. The behavior with attachments seems OK (would be nicer to highlight new attachments with the green border, but it's not a big deal). It's a bit sad we need to transmit all comments each time, as this means this will generate a lot more traffic. The positive side effect though is that this will correctly show the deletions that might have happened concurrently.

The last point of comment:30 is still opened but I understand you'd like to apply the current state as an intermediate step, which is fine by me.

(PS: s/controled/controlled/, that's all what you can get from me for a code review, these days ;-) )

[Remy Blank]

Replying to cboos:

I tested the patch, I think it's OK now.

Thanks for testing!

The behavior with attachments seems OK (would be nicer to highlight new attachments with the green border, but it's not a big deal).

I'll see if I can achieve that easily.

It's a bit sad we need to transmit all comments each time, as this means this will generate a lot more traffic. The positive side effect though is that this will correctly show the deletions that might have happened concurrently.

Yes, there's more traffic, but we have to generate the data for all comments anyway, so at least it's not more processing. Other positive side-effects include:

• "in reply to" and "follow-up" links are updated.
• Comment times are updated.

If the additional traffic is too high, I have a few ideas for optimization:

• Include the "last change time" in the XHR and return all comments only if a new comment or attachment was added (i.e. the changetime of the ticket has changed).
• Switch to a different model, like they do on stackoverflow.com: send a periodic request for "any new comments added" (i.e. a boolean), and display a message if there are, inviting the user to click "Preview".

I think there's room for improvement here, but I wanted to get the functionality working first.

The last point of comment:30 is still opened but I understand you'd like to apply the current state as an intermediate step, which is fine by me.

Yes, that was the intention.

[Christian Boos]

Replying to rblank:

The last point of comment:30 is still opened but I understand you'd like to apply the current state as an intermediate step, which is fine by me.

Yes, that was the intention.

One additional idea for this point: it would be great to highlight the conflicts. For example, say you changed the priority from normal to high, while an in between modification changed it to low, you'd then get to see:

• Priority changed from normal to high (conflicts with low)

That line could even end with a [revert] inline button, restoring the value low set by the other change.

[Remy Blank]

7145-concurrent-editing-r10684.patch​ applied in [10687]. Good idea in comment:35!

[Remy Blank]

Highlight field conflicts and show busy indicator.

[Remy Blank]

7145-conflicts-busy-r10688.patch​ adds highlighting of field conflicts and shows a busy indicator while fetching updates for ticket changes, ticket comment edits as well as new tickets.

A field change is highlighted as conflicting if it was changed since starting the modification. In particular, a field that was changed to some value, and back to the original value, is still marked as a conflict. I believe this is better than the alternative, because it forces the user to notice and think about the change. It's also a relatively rare situation.

Feedback welcome.

There's no [revert] button yet, so that would be the next step. I think this is a better solution than automatically updating field editors that haven't been modified by the user (comment:29), as the latter could lead to "race conditions" with the user, e.g. if she had the cursor placed in a text input, ready to edit something, and the field was updated.

[Christian Boos]

Funny: the notice saying: and any conflicts shown in the preview below part is exactly what I had in mind ;-)

For the conflict report on a field, the changes shown are between the original value and the locally modified value, but the other change is not shown (see my example in comment:35). But if you wanted to add something like the (conflicts with …) part together with the [revert] button, fine with me.

Another missing piece I think we should add are the diffs for text field changes.

But the current patch is already a nice step forward as it is.

[Remy Blank]

Replying to cboos:

For the conflict report on a field, the changes shown are between the original value and the locally modified value, but the other change is not shown (see my example in comment:35).

The changes shown are between the current (remotely-modified) value and the locally-modified value. I actually missed the (conflicts with …) part in comment:35 (which contradicts my definition of a conflict, as a field modified to a different value and back to the original value should not result in a conflict in this case).

I had an implementation that would have allowed displaying the conflicting value. But I wonder if it's the right functionality. When you have a conflict, you have one of three situations:

• You want to keep your local change, in which case you have nothing else to do.
• You want to keep the new (remotely-modified) value, in which case you revert your local change with the [revert] button.
• You want to set a different value altogether, in which case you just set the new value.

Both the new and locally-changed values are shown already (except for textarea fields), so I don't see a need for the (conflicts with …) part. The conflict markers are there to indicate to the user which field changes she has to check in the new changes (marked in green).

We could even add the [revert] button to all field changes (even non-conflicting), to easily allow reverting an edit.

Another missing piece I think we should add are the diffs for text field changes.

Yes, those would be nice indeed, even in the non-concurrent edit situation.

[Christian Boos]

Replying to rblank:

The changes shown are between the current (remotely-modified) value and the locally-modified value.

My mistake.

I actually missed the (conflicts with …) part in comment:35 (which contradicts my definition of a conflict, as a field modified to a different value and back to the original value should not result in a conflict in this case).

Ok, this case is really the heart of the "problem". In comment:37, you note that this should be a rare situation, but I think it's on the contrary the most common, as it would happen for any field that has been modified remotely but not locally.

My problem is that currently this would be displayed the same way as a real conflict. The following table shows the different situations:

	Old	Local	Remote
0	a	a	a
1	a	b	a
2	a	a	c
3	a	b	c
4	a	b	b

0) no changes on either side, nothing shown (added for completeness) 1) the usual no conflict case 2) the kind of conflict you'd get if you changed nothing 3) the "real" conflict of simultaneous conflicting change to the same field 4) no conflict, no change between local and remote, so nothing should be shown

For 1) we show:

• <field> changed from a to b

For 2) and 3) we show:

• | <field> changed from c to a (-)
• | <field> changed from c to b (-)

If we really want to show 2), then I'd suggest to show it in a special way, e.g.

• | <field> reverted from c to a (-)

But I wonder if we should really show it at all, as it's really not a conflict.

[Remy Blank]

Replying to cboos:

In comment:37, you note that this should be a rare situation, but I think it's on the contrary the most common, as it would happen for any field that has been modified remotely but not locally.

The situation I meant was rare is where a field is updated remotely to some value, and again remotely to the initial value. The situation where a field is updated remotely but unchanged locally is indeed the most common situation (well, maybe an even more common situation is where the remote change only adds a comment, without modifying any fields).

(snip fully correct analysis)

For 2) and 3) we show:

• | <field> changed from c to a (-)
• | <field> changed from c to b (-)

If we really want to show 2), then I'd suggest to show it in a special way, e.g.

• | <field> reverted from c to a (-)

But I wonder if we should really show it at all, as it's really not a conflict.

Agreed, it's not a conflict. But in the current state, it's still a field for which the user has to make a decision, that's why I have marked it as a conflict.

The only way I see to avoid even showing 2) is to automatically update the relevant field editors, something I am not comfortable with due to the "race condition" with the user I mentioned in comment:37. My feeling is that the [revert] button makes it easy enough to fix it, while avoiding the race condition.

[Christian Boos]

Replying to rblank:

Replying to cboos:

In comment:37, you note that this should be a rare situation, but I think it's on the contrary the most common, as it would happen for any field that has been modified remotely but not locally.

The situation I meant was rare is where a field is updated remotely to some value, and again remotely to the initial value.

OK, I see now.

For 2) and 3) we show:

• | <field> changed from c to a (-)
• | <field> changed from c to b (-)

If we really want to show 2), then I'd suggest to show it in a special way, e.g.

• | <field> reverted from c to a (-)

But I wonder if we should really show it at all, as it's really not a conflict.

Agreed, it's not a conflict. But in the current state, it's still a field for which the user has to make a decision, that's why I have marked it as a conflict.

Well, the user should be able to notice that in the "green" zone, but …

The only way I see to avoid even showing 2) is to automatically update the relevant field editors, something I am not comfortable with due to the "race condition" with the user I mentioned in comment:37. My feeling is that the [revert] button makes it easy enough to fix it, while avoiding the race condition.

I agree, this is perhaps the best pragmatic solution. We could eventually prevent this race by disabling the controls during the preview round trip, but I imagine this will make the interaction painful.

So would you be OK for at least the "reverted from"… proposal?

[Remy Blank]

Replying to cboos:

So would you be OK for at least the "reverted from"… proposal?

Sure, I'll give it a try. It makes the code a bit more complicated, because you have to reconstruct the field contents as of start_time instead of just keeping track of which fields have changed.

We could also use a different color for this case, but maybe that's too much.

[Christian Boos]

Feel free to commit the current patch as an intermediate step (and I'll update ;-) ).

[Remy Blank]

7145-conflicts-busy-r10688.patch​ committed in [10690] (and a small cosmetic fix in [10691]).

[Remy Blank]

comment:2:ticket:10207 suggests restarting the auto-preview timer on mouse move events, to prevent the update from shifting the "Preview" and "Submit changes" while the user is trying to hit either of them.

[franz.mayer@…]

This feature is really valuable and was already requested from some collegues of mine. The implemented version is already a good step forward, thanks!

Yet I have a few notes to this feature:

• Error messages and notifications are spread over the whole page (one error message at top, another above comments field, and the conflict list at the very bottom) —> merge the messages at one place (maybe at top)
• When a conflict is present, there is a list with all conflicts, but you cannot diff it —> it would be nice, if you could diff the difference (without loosing the changes made) and decide which versions you would like to adopt

[franz.mayer@…]

Another thing, which I noticed while testing this issue: When a user just adds a comment at a ticket, there should not be any hint / warning about ticket modifications, because it distracts users much more than it helps.

[Christian Boos]

We just had it again on #10515, so let me revisit the topic with what I think would be a simple solution.

I got (with t.e.o. at r10723):

• Reporter changed from mike_mp@zzzcomputing.com to anonymous
• Version changed from 0.12.2 to 0.12dev

The second entry is fine (it is actually a "conflict", you did "" -> "0.12.2", I did "" -> "0.12dev"), but we should also really have a "Revert" or "(-)" button there ;-)

The first entry is not, as I did nothing ("" -> "") and Rémy did "" -> "mike_mp@", so it should rather be not taken as a conflict, we should keep the other changes and not be displayed at all, as it's already reported in the "green zone".

[osimons]

Hmm. [10687] has broken ​th:XmlRpcPlugin ticket update timestamp handling. The plugin supports mid-air collision detection by injecting values into req.args required by validation, but the changeset makes 2 vital changes that I have no way to detect in a compatible way:

1. The variable name changes from 'ts' to 'view_time'.
2. The format changes from str(ticket['changetime']) to str(to_utimestamp(ticket['changetime']))

Are the changes required, or can it be reverted to use the old names and format?

Unfortunately all ticket validation code is stuck in ticket.web_ui module, so the RPC plugin is very much affected by HTML changes. Which is by no means a good thing - but a topic for another time… ;-)

[osimons]

Revert from view_time back to ts and also keep original readable timestamp format.

[osimons]

Reverted timestamp handling in attachment:t7145-revert_to_ts-r11018.diff​ and it seems to work in my testing.

OK to apply?

[Remy Blank]

Replying to osimons:

OK to apply?

No, not ok. Passing the time as a default-formatted datetime was a mistake in my initial commit, it should always have been a timestamp. And IIRC I changed the name view_time because it was misleading: it's not the view time, it's the changetime at the time the user got his last data batch.

Can you please explain why you can't detect the change in a compatible way?

[osimons]

Replying to rblank:

Can you please explain why you can't detect the change in a compatible way?

​th:ticket:9921 probably explains that best, and the patch in comment 1 shows how I need to insert both as I can't detect from ticket.web_ui which variable name may be used by installed Trac version. And, changing just the internal format but keeping the name would be an even worse option.

[Remy Blank]

Ok, looking more closely at the code, I got that backwards. I actually replaced ts with view_time, and not the other way around. So it is slightly misleading, and should probably be called view_changetime. I don't really remember the exact reasons for the rename, probably I found ts too generic and prone to collision, and I already had start_time so view_* seemed about right (ts… time stamp? time stamp of what?).

I'm not familiar with your plugin, but can't you just set both ts and view_time, each with the correct format?

[Remy Blank]

Allow reverting individual fields changes.

[Remy Blank]

7145-revert-field-r11053.patch​ allows reverting individual fields of a ticket edit, using a "(revert)" link next to each modified field. I believe it works correctly in the most frequent configurations and corner cases. In particular:

• It works both with and without JavaScript.
• It correctly reverts all field types (text, textarea, select, checkbox, radio).
• It handles the special case where CC is a checkbox, and shouldn't leak email addresses in that case.
• The "(revert)" links are displayed next to every field change in the preview (except the ones due to the workflow), whether they have conflicts or not. I find this very useful, for example when I edit the description, but later want to revert the edit before submitting.
• The JavaScript version works with all the browsers that I could lay a hand on (Firefox 10, Opera 12, Chromium 19, IE 9, Safari).

For the non-JavaScript version to work, it was necessary to use a (submit) button instead of a link to trigger the revert, so that the form gets submitted. I re-styled the buttons so that they look like the "(diff)" links. To make this easier, I used <button> instead of <input> so that I didn't have to revert all the border and shadow stuff in the CSS. If there's a preference for <input> (we didn't have any <button> up to now), I can also use that instead, it just makes the CSS uglier.

I believe this patch makes the conflict handling functionality reasonably complete and easy to use, without introducing any race conditions with the user editing the fields. Feedback welcome. From my side, I'd be ready to commit.

[Christian Boos]

Looks great … can't wait to test it but that will be for tomorrow, I'm afraid ;-)

[Christian Boos]

I now started testing it (Chrome 19) and it works very well.

One thing which I find a bit disturbing is the delay when clicking on the (revert) button. I understand that we apply the usual preview delay here as we don't want to send to many update events in case there are several reverts, but perhaps the line which is reverted should at least disappear right away?

Also, concerning the look of this remove link, I think a (-) button on the left, like we have for the custom query filters, could look better (haven't tried yet).

I also haven't tried CC as checkbox yet, but for the normal CC: as text input, I noticed a non optimal handling, due to our normalization of the field w.r.t. to the value separator (i.e. from "a b c" to "a, b, c"). If that normalization could be done before the comparison, we would have a more meaningful change report:

• A and B start from empty
• A inserts "a b"
• B inserts "b c"
• A saves
• B sees: b c added; a, b removed

If B had written "b, c" (or if that normalization would have been done beforehand), he (Bob ;-) ) would have seen: c added; b removed

Finally, for textarea fields I see that we only have the mention of a conflict, but no way to see what the conflict actually is, even less the possibility to do some merge operation. It would be nice to be able to do so, but that will probably have to be done in a follow-up ticket.

[Christian Boos]

I implemented most of the above in repos:cboos.git:ticket7145/cc-extras:

• [5eb0d11b/cboos.git] 7145-revert-field-r11053.patch
• [5f8a1487/cboos.git] #7145: white-space fixes on previous patch
• [cdc6800a/cboos.git] ticket preview: normalize new CC: field to improve display of changes
• [40cd945a/cboos.git] #7145: use a rounded (-) revert button at the left of the change line
• [f29f0206/cboos.git] #7145: Javascript adaptations to the revert button changes
• [23676875/cboos.git] #7145: immediately remove the change preview after reversal

I also noticed that we indeed have a (diff) link for the description changes, but in the ticket box preview, not in the ticket change line. And that would be anyway for "mine" diffs, not for "theirs" diffs. The best solution would probably be to add (diff mine), (diff theirs) and (merge) actions in the ticket change panel. Clicking on such actions would expand the corresponding content, including a conflict resolution panel (not for 1.0 though!)

[Christian Boos]

The above changes were not enough, as the alignment was not uniform when different parameters changed (conflict or not, user change or not).

[3dfec068/cboos.git] improves this, and here you can see how it looks like in different browsers (from left to right, Opera 11.64, FF 13.0.1, IE9, Chrome 20.0.1132.39):

[Christian Boos]

combined screenshot of the revert change buttons at [3dfec068/cboos.git]

[Remy Blank]

Replying to cboos:

One thing which I find a bit disturbing is the delay when clicking on the (revert) button. I understand that we apply the usual preview delay here as we don't want to send to many update events in case there are several reverts, but perhaps the line which is reverted should at least disappear right away?

I tried this initially, and didn't really like it. But I don't have strong feelings about it, and re-trying it today, it's quite ok. So +1.

Also, concerning the look of this remove link, I think a (-) button on the left, like we have for the custom query filters, could look better (haven't tried yet).

This, OTOH, I don't like at all :) I find the link clearer, less visually cluttering, and consistent with the "(diff)" link.

I also haven't tried CC as checkbox yet, but for the normal CC: as text input, I noticed a non optimal handling, due to our normalization of the field w.r.t. to the value separator (i.e. from "a b c" to "a, b, c").

Yeah, CC is a mess, and is special-cased in far too many locations. This should really be a list-type field, or barring that we should at least do the normalization in the model. But it's not the right moment for such a refactoring, so your fix is fine.

Replying to cboos:

I also noticed that we indeed have a (diff) link for the description changes, but in the ticket box preview, not in the ticket change line.

There's actually both. The "(diff)" link above the description in the box at the top shows the diff for the last change of the description. But there's also a "(diff)" link next to the text "Description modified" in the change history every time the description (or any "textarea" custom field, for that matter) is modified.

And that would be anyway for "mine" diffs, not for "theirs" diffs. The best solution would probably be to add (diff mine), (diff theirs) and (merge) actions in the ticket change panel. Clicking on such actions would expand the corresponding content, including a conflict resolution panel (not for 1.0 though!)

Please, no… This smells like feature creep. Do you really want 3-way merging for ticket fields? I would understand for wiki pages, but how often do two people make significant (i.e. multi-paragraph) changes to the description of the same ticket simultaneously? Let's not make this more complicated than necessary.

7145-revert-field-r11072.patch​ is the combined patch with your changes, except for the button.

[Remy Blank]

Added improved CC handling and immediate removal.

[Christian Boos]

Replying to rblank:

Replying to cboos:

Also, concerning the look of this remove link, I think a (-) button on the left, like we have for the custom query filters, could look better (haven't tried yet).

This, OTOH, I don't like at all :) I find the link clearer, less visually cluttering, and consistent with the "(diff)" link.

By visual clutter you mean the colors? I can make this more discrete (like in the initial change [40cd945a/cboos.git]). Otherwise it's less text ((-) vs. (revert)) and the consistent placement makes it easier to find than at the end of lines of varying length. To other selling point for that layout is the consistency with the custom filters, as those buttons do exactly the same: remove the line. The consistency with the (diff) link is not really appropriate, because it's not a link and doesn't behave like one. If we can't find a common ground with the placement at the left, at least consider using the inlinebuttons style like we have for the other actions.

But in fact, I also went in that direction because I'd like to reduce the Reply, Edit, Delete buttons to more "iconic" ones, with just an arrow (→), (…) and (-) (or (x)).

Replying to cboos:

I also noticed that we indeed have a (diff) link for the description changes, but in the ticket box preview, not in the ticket change line.

There's actually both. The "(diff)" link above the description in the box at the top shows the diff for the last change of the description. But there's also a "(diff)" link next to the text "Description modified" in the change history every time the description (or any "textarea" custom field, for that matter) is modified.

Ok, so we have those links on the page, but they're scattered. I think it would be nice to have a repetition of these links here, as (diff mine) (diff theirs), in case of a conflict.

… and (merge) actions in the ticket change panel. Clicking on such actions would expand the corresponding content, including a conflict resolution panel (not for 1.0 though!)

Please, no… This smells like feature creep. Do you really want 3-way merging for ticket fields? I would understand for wiki pages, but how often do two people make significant (i.e. multi-paragraph) changes to the description of the same ticket simultaneously? Let's not make this more complicated than necessary.

Sure, this is a bit too far fetched (yet… reconciling two different lines of a ticket in the DistributedTrac scenario… ;-) ).

7145-revert-field-r11072.patch​ is the combined patch with your changes, except for the button.

[Remy Blank]

Replying to cboos:

By visual clutter you mean the colors? I can make this more discrete (like in the initial change [40cd945a/cboos.git]). Otherwise it's less text ((-) vs. (revert)) and the consistent placement makes it easier to find than at the end of lines of varying length.

No, it's the fact that I find it better to have the important information first (leftmost). Reverting is a relatively rare operation, compared to reading the change info. Putting the revert link at the end makes it more discrete.

To other selling point for that layout is the consistency with the custom filters, as those buttons do exactly the same: remove the line.

Custom filters are not on this page, so I don't see a need to be consistent with them.

If we can't find a common ground with the placement at the left, at least consider using the inlinebuttons style like we have for the other actions.

That will make the lines showing changes much higher, and it looks weird. I know, it was the first thing I tried :) To avoid this effect, you have to make the buttons very small, so they are more difficult to hit, and then I find they look strange (we don't have such small buttons anywhere else in Trac). Links, OTOH, we have plenty of them.

I know that in some distant past, there was a convention that links were for navigating, and buttons for actions. Most sites don't make this distinction anymore, and there wasn't much justification to have that rule in the first place. We have broken it in several places already, for example we have titles (links) that expand a section when clicked (an action).

[Christian Boos]

Ok, so I did my own little usability testing here, showing the following variants:

1. 
2. 
3. 
4. 

2. was deemed to be confusing ("does this expand something?") :-(
3. the meaning of the - button on the right was not immediately clear either
4. and 1. were the preferred ones; 4. "a bit more modern", 1. "also fine, feels even Tracier or Wikier (cf. Wikipedia View history (undo) (*))"

The thing is that as one guideline for #10012, I really would have liked to use buttons and inline buttons more consistently, for things that were not links. So my preference would be 4., but 1. is also fine by me, in the end. Your call ;-)

(*) there at least it's really a link which goes to an "undo" form

[Christian Boos]

Remy's proposal, (revert) "link" on the right

[Christian Boos]

• button on the left, in place of the bullet

[Christian Boos]

• button on the right

[Christian Boos]

[revert] button on the right

[Remy Blank]

Heh, you're doing this thoroughly :) I'll stay with the initial design, because of the very small text in the buttons in 4 (and it makes the line height larger in my browser, because I set a larger minimum text size), and because I find it looks strange when buttons are zig-zagging (whereas the links flow with the text).

I noticed one glitch with the immediate removal: if you revert one field, then revert another while the auto-preview is fetching (the wheel is spinning), then the second field re-appears due to the auto-preview, and re-disappears at the next cycle. I think that's ok, the immediate feedback of the removal has enough value that we can tolerate this small glitch.

Patch applied in [11077].

[Christian Boos]

Yay, great! Thanks Rémy for bearing with my nitpicking ;-)