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.

[cboos]

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.

[cboos]

Milestone 1.0 deleted

[rblank]

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?

[rblank]

Improved ticket change preview.

[rblank]

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.

[rblank]

Fully dynamic ticket creation and edition.

[rblank]

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?

[rblank]

The patch is against trunk at [10560].

[cboos]

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 ;-)

[rblank]

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 :)

[rblank]

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

[rblank]

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.

[rblank]

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

[rblank]

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.

[rblank]

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.

[rblank]

Improve handling of ticket comment collisions.

[rblank]

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.

[cboos]

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;
  

[cboos]

example of timeline's style left green border

[cboos]

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").

[rblank]

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.

[cboos]

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

[rblank]

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.

[rblank]

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

[rblank]

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.

[rblank]

Improved concurrent editing.

[rblank]

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!

[cboos]

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.

[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.

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.

[cboos]

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

[cboos]

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.

[rblank]

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.

[rblank]

Fixed most issues.

[rblank]

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.

[rblank]

Updated patch for current trunk.

[cboos]

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 ;-) )

[rblank]

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.

[cboos]

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.

[rblank]

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

[rblank]

Highlight field conflicts and show busy indicator.

[rblank]

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.

[cboos]

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.

[rblank]

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.

[cboos]

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.

[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. 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.

[cboos]

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?

[rblank]

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.

[cboos]

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

[rblank]

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

[rblank]

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.

[cboos]

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".