Edgewall Software
Modify

Opened 6 years ago

Closed 22 months ago

Last modified 22 months ago

#7145 closed enhancement (fixed)

Improve handling of conflicts due to concurrent ticket edits

Reported by: anonymous Owned by: rblank
Priority: high Milestone: 1.0
Component: ticket system Version: 0.10.4
Severity: normal Keywords: concurrent edit review
Cc: mmitar@…, osimons, kirean@…, franz.mayer@…
Release Notes:

Improved the handling of ticket edit conflicts, by showing simultaneous edits and conflicting changes, and making it easier to merge the changes.

API Changes:

Description

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.

Attachments (19)

7145-ticket-change-preview-r10500.patch (9.7 KB) - added by rblank 3 years ago.
Improved ticket change preview.
7145-ticket-preview-r10560.patch (18.4 KB) - added by rblank 3 years ago.
Fully dynamic ticket creation and edition.
7145-ticket-preview-2-r10560.patch (17.7 KB) - added by rblank 3 years ago.
Same patch, but ticket comment preview is right below the comment editor.
7145-ticket-preview-r10632.patch (18.9 KB) - added by rblank 3 years ago.
Comment preview back on top of properties, but property section collapses on preview if the focus is in the comment editor.
7145-concurrent-editing-r10647.patch (22.4 KB) - added by rblank 3 years ago.
Improve handling of ticket comment collisions.
t7145-trac-new.png (10.4 KB) - added by cboos 3 years ago.
example of timeline's style left green border
7145-concurrent-editing-2-r10647.patch (25.0 KB) - added by rblank 3 years ago.
Improved concurrent editing.
t7145-follow-up-comment28.diff (6.7 KB) - added by cboos 3 years ago.
a few follow-ups to attachment:7145-concurrent-editing-2-r10647.patch
7145-concurrent-editing-r10668.patch (43.8 KB) - added by rblank 3 years ago.
Fixed most issues.
7145-concurrent-editing-r10684.patch (45.7 KB) - added by rblank 3 years ago.
Updated patch for current trunk.
7145-conflicts-busy-r10688.patch (9.5 KB) - added by rblank 3 years ago.
Highlight field conflicts and show busy indicator.
t7145-revert_to_ts-r11018.diff (2.7 KB) - added by osimons 2 years ago.
Revert from view_time back to ts and also keep original readable timestamp format.
7145-revert-field-r11053.patch (9.6 KB) - added by rblank 23 months ago.
Allow reverting individual fields changes.
revert-button-3dfec068.png (34.2 KB) - added by cboos 22 months ago.
combined screenshot of the revert change buttons at [3dfec068/cboos.git]
7145-revert-field-r11072.patch (10.7 KB) - added by rblank 22 months ago.
Added improved CC handling and immediate removal.
7145-revert_revert_link_right.png (8.7 KB) - added by cboos 22 months ago.
Remy's proposal, (revert) "link" on the right
7145-revert_-_left.png (8.7 KB) - added by cboos 22 months ago.
- button on the left, in place of the bullet
7145-revert_-_right.png (8.8 KB) - added by cboos 22 months ago.
- button on the right
7145-revert_revert_right.png (8.7 KB) - added by cboos 22 months ago.
[revert] button on the right

Download all attachments as: .zip

Change History (84)

comment:1 follow-up: Changed 6 years ago by cboos

  • Milestone changed from 0.11 to 1.0

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

comment:2 in reply to: ↑ 1 Changed 6 years ago by 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.

comment:3 Changed 4 years ago by cboos

  • Milestone changed from 1.0 to unscheduled

Milestone 1.0 deleted

comment:4 Changed 4 years ago by rblank

  • Milestone changed from triaging to 0.13
  • Owner changed from cboos to 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.

comment:5 Changed 4 years ago by rblank

  • Priority changed from normal to high

comment:6 Changed 4 years ago by Mitar

  • Cc mmitar@… added

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?

Changed 3 years ago by rblank

Improved ticket change preview.

comment:7 Changed 3 years ago by 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.

Changed 3 years ago by rblank

Fully dynamic ticket creation and edition.

comment:8 Changed 3 years ago by 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.

comment:9 Changed 3 years ago by Mitar

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

comment:10 Changed 3 years ago by rblank

The patch is against trunk at [10560].

comment:11 follow-up: Changed 3 years ago by 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 ;-)

comment:12 in reply to: ↑ 11 Changed 3 years ago by 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 :)

Changed 3 years ago by rblank

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

comment:13 Changed 3 years ago by 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.

comment:14 Changed 3 years ago by cboos

  • Keywords review added

Changed 3 years ago by rblank

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

comment:15 Changed 3 years ago by 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.

comment:16 Changed 3 years ago by 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.

comment:17 Changed 3 years ago by osimons

  • Cc osimons added

Changed 3 years ago by rblank

Improve handling of ticket comment collisions.

comment:18 Changed 3 years ago by 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.

comment:19 Changed 3 years ago by 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;
    

example of timeline's style left green border

Changed 3 years ago by cboos

example of timeline's style left green border

comment:20 Changed 3 years ago by 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").

comment:21 Changed 3 years ago by kirean@…

  • Cc kirean@… added

comment:22 follow-up: Changed 3 years ago by 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.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 3 years ago by cboos

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

comment:24 in reply to: ↑ 23 Changed 3 years ago by 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.

comment:25 Changed 3 years ago by rblank

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

comment:26 Changed 3 years ago by 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.

Changed 3 years ago by rblank

Improved concurrent editing.

comment:27 Changed 3 years ago by 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!

Last edited 3 years ago by rblank (previous) (diff)

comment:28 follow-up: Changed 3 years ago by 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

    diff -r 9ce137a90d5f trac/htdocs/js/threaded_comments.js
    a b jQuery(document).ready(function($){ 
    22  var comments = null; 
    33  var toggle = $('#trac-threaded-toggle'); 
    44  toggle.click(function() { 
    5     if ($(this).checked()) { 
     5    $(this).toggleClass('checked'); 
     6    if ($(this).hasClass('checked')) { 
    67      comments = $("div.change"); 
    78      comments.each(function() { 
    89        var children = $("a.follow-up", this).map(function() { 
  • trac/ticket/templates/ticket.html

    diff -r 9ce137a90d5f trac/ticket/templates/ticket.html
    a b  
    4444          // Unthread and update changelog 
    4545          var threaded_toggle = $('#trac-threaded-toggle'); 
    4646          if (threaded_toggle.checked()) 
    47             threaded_toggle.attr('checked', false).click().attr('checked', false); 
     47            threaded_toggle.click(); 
    4848          $("#changelog .trac-new").remove(); 
    4949          var new_changes = items.filter("#new-changes").children(); 
    5050          $("#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

    a b  
    4444          // Unthread and update changelog 
    4545          var threaded_toggle = $('#trac-threaded-toggle'); 
    4646          if (threaded_toggle.checked()) 
    47             threaded_toggle.attr('checked', false).click().attr('checked', false); 
     47            threaded_toggle.click(); 
    4848          $("#changelog .trac-new").remove(); 
    4949          var new_changes = items.filter("#new-changes").children(); 
    5050          $("#changelog").append(new_changes); 
     
    160160          </h2> 
    161161          <div id="trac-edit-warning" class="warning system-message" 
    162162               style="${'display: none' if start_time == ticket['changetime'] else None}"> 
    163             This ticket has been modified since you started editing. The 
    164             <span class="trac-new">other modifications</span> are highlighted. 
     163            This ticket has been modified since you started editing. You should review the 
     164            <em class="trac-new">other modifications</em> which have been appended above. 
     165            You can nevertheless proceed and submit your changes if you wish so. 
    165166          </div> 
    166167          <!--! Comment field --> 
    167168          <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

    a b div.comment ol { list-style: decimal } 
    110110} 
    111111#trac-comment-editor .wikitoolbar { margin-left: -1px } 
    112112#trac-add-comment :link, #trac-add-comment :visited { color: #b00 } 
    113 #changelog .trac-new { border-left: 0.3em solid #c0f0c0; padding-left: 0.3em; } 
     113.trac-new { border-left: 0.31em solid #c0f0c0; padding-left: 0.31em; } 
    114114#changelog h3, #ticketchange h3 { 
    115115 border-bottom: 1px solid #d7d7d7; 
    116116 color: #999; 
    form .field .wikitoolbar { margin-left: 
    138138form .field div.trac-resizable { width: 100% } 
    139139 
    140140#propertyform { margin-bottom: 2em; } 
    141 #trac-edit-warning .trac-new { background-color: #c0f0c0; } 
    142141#properties { white-space: nowrap; line-height: 160%; padding: .5em } 
    143142#properties table { border-spacing: 0; width: 100%; padding: 0 .5em } 
    144143#properties table th { 

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

Wrap-up patch upcoming.

comment:29 in reply to: ↑ 28 ; follow-up: Changed 3 years ago by 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.

comment:30 in reply to: ↑ 29 Changed 3 years ago by 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.

comment:31 Changed 3 years ago by 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.

Changed 3 years ago by rblank

Fixed most issues.

comment:32 Changed 3 years ago by 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.

Changed 3 years ago by rblank

Updated patch for current trunk.

comment:33 follow-up: Changed 3 years ago by 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 ;-) )

comment:34 in reply to: ↑ 33 ; follow-up: Changed 3 years ago by 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.

comment:35 in reply to: ↑ 34 Changed 3 years ago by 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.

Changed 3 years ago by rblank

Highlight field conflicts and show busy indicator.

comment:37 Changed 3 years ago by 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.

comment:38 follow-up: Changed 3 years ago by 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.

comment:39 in reply to: ↑ 38 ; follow-up: Changed 3 years ago by 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.

comment:40 in reply to: ↑ 39 ; follow-up: Changed 3 years ago by 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.

comment:41 in reply to: ↑ 40 ; follow-up: Changed 3 years ago by 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.

comment:42 in reply to: ↑ 41 ; follow-up: Changed 3 years ago by 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?

comment:43 in reply to: ↑ 42 Changed 3 years ago by 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.

comment:44 Changed 3 years ago by cboos

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

comment:45 Changed 3 years ago by rblank

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

comment:46 Changed 3 years ago by 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.

comment:47 Changed 3 years ago by 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

comment:48 Changed 2 years ago by franz.mayer@…

  • Cc franz.mayer@… added

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.

comment:49 Changed 2 years ago by 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".

comment:50 Changed 2 years ago by 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… ;-)

Changed 2 years ago by osimons

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

comment:51 follow-up: Changed 2 years ago by osimons

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

OK to apply?

comment:52 in reply to: ↑ 51 ; follow-up: Changed 2 years ago by rblank

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?

comment:53 in reply to: ↑ 52 Changed 2 years ago by 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.

comment:54 Changed 2 years ago by rblank

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?

Changed 23 months ago by rblank

Allow reverting individual fields changes.

comment:55 Changed 23 months ago by rblank

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.

comment:56 Changed 22 months ago by cboos

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

comment:57 follow-up: Changed 22 months ago by cboos

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.

comment:58 follow-up: Changed 22 months ago by cboos

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

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

comment:59 in reply to: ↑ 58 Changed 22 months ago by cboos

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): combined screenshot of the revert change buttons at [3dfec068/cboos.git]

Changed 22 months ago by cboos

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

comment:60 in reply to: ↑ 57 ; follow-up: Changed 22 months ago by rblank

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.

Changed 22 months ago by rblank

Added improved CC handling and immediate removal.

comment:61 in reply to: ↑ 60 ; follow-up: Changed 22 months ago by cboos

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.

comment:62 in reply to: ↑ 61 Changed 22 months ago by rblank

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

comment:63 Changed 22 months ago by cboos

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

  1. Remy's proposal, (__revert__) "link" on the right
  2. - button on the left, in place of the bullet
  3. - button on the right
  4. [revert] button on the right
  1. was deemed to be confusing ("does this expand something?") :-(
  2. the meaning of the - button on the right was not immediately clear either
  3. 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

Last edited 22 months ago by cboos (previous) (diff)

Changed 22 months ago by cboos

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

Changed 22 months ago by cboos

  • button on the left, in place of the bullet

Changed 22 months ago by cboos

  • button on the right

Changed 22 months ago by cboos

[revert] button on the right

comment:64 Changed 22 months ago by rblank

  • Release Notes modified (diff)
  • Resolution set to fixed
  • Status changed from new to closed

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

comment:65 Changed 22 months ago by cboos

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed The owner will remain rblank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from rblank to the specified user.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.