Allow change comment edit after commit.

[willow]

Often when browsing changes in the wiki I see undescribed comments. I'd like to be able to add them after the commit to make the history of a page more readable. I think this function would be useful from the "Changes between Version # and Version # of wikipage" page.

[Emmanuel Blot]

0.10.5 will be a maintenance release (for security fixes mostly) and won't support new features.

[Christian Boos]

Related to #454.

[Christian Boos]

Milestone 1.0 deleted

[Christian Boos]

Metadata editing.

[john.ferg@…]

If I could vote for this one I would. I would find this extremely useful.

[Peter Suter]

I would find this useful as well, so I tried to implement it in the attached patch.

• Does reusing the WIKI_ADMIN permission make sense?
• Should a change listener be called? If yes, call the existing wiki_page_changed method or a new one?
• I first tried to use an Edit button instead of a link:

                  <div class="inlinebuttons">
                    <input type="hidden" name="action" value="edit_comment" />
                    <input type="hidden" name="version" value="$item.version"/>
                    <input type="submit" value="${captioned_button('✎', _('Edit'))}" title="Edit comment" />
                  </div>
  

  But this seems to be impossible / conflict with the existing form in the table. Any better ideas?

[Ryan J Ollos]

Replying to psuter:

I would find this useful as well, so I tried to implement it in the attached patch.

It will be great to have this feature added!

• Does reusing the WIKI_ADMIN permission make sense?

Yeah, I think WIKI_ADMIN is the most logical permission in absence of a wiki comment history.

• Should a change listener be called? If yes, call the existing wiki_page_changed method or a new one?

It would probably be good to call a change listener. Consistency with whatever is implemented for #11377 would be good. The changes in [2a5bf424/jomae.git] look good to me, which would suggest wiki_page_comment_modified(page, cdate, author, comment, old_comment).

• I first tried to use an Edit button instead of a link:

                  <div class="inlinebuttons">
                    <input type="hidden" name="action" value="edit_comment" />
                    <input type="hidden" name="version" value="$item.version"/>
                    <input type="submit" value="${captioned_button('✎', _('Edit'))}" title="Edit comment" />
                  </div>
  

  But this seems to be impossible / conflict with the existing form in the table. Any better ideas?

Yeah, I don't see any way around that. Is there an advantage to using a button rather than a link for this case?

I was reminded of #11130. I like the styling of the Edit links on the Available Reports page, and came up with:

I thought the placement of the Edit links was easier to notice with them located next to the comment. Floating them to the right could be okay too, but they might not be as easily noticed.

I worked off your patch in log:rjollos.git:t6573.

[Peter Suter]

Replying to rjollos:

I worked off your patch in log:rjollos.git:t6573.

Thanks, your changes look good to me.

If you have time for some more style cleanups, check the sources I copied from:

• wiki_rename.html​ (missing spaces before />)
• model.py​ ('' instead of "")
• web_ui.py​ (a lot of _(' instead of _(").

It would probably be good to call a change listener. Consistency with whatever is implemented for #11377 would be good. The changes in [2a5bf424/jomae.git] look good to me, which would suggest wiki_page_comment_modified(page, cdate, author, comment, old_comment).

I think at least version should be included, and cdate and author might not be needed here: wiki_page_comment_modified(page, version, comment, old_comment).

Is there an advantage to using a button rather than a link for this case?

Probably not. (I just followed the example of ticket comment buttons.)

Should ${captioned_button('✎', _('Edit'))} be used with the link as well?

[Jun Omae]

Replying to psuter:

Should ${captioned_button('✎', _('Edit'))} be used with the link as well?

That's good to me.

Also, I think the link buttons should have hover and active effects.

trac/htdocs/css/trac.css

@@ -109 +109 @@
  text-shadow: .1em .1em #fcfcfc;
 }
 input[type=button]:active, input[type=submit]:active,
-input[type=reset]:active {
+input[type=reset]:active, .inlinebuttons a:active {
  position: relative;
  top: .1em;
  left: .1em;
@@ -171 +171 @@
  cursor: pointer;
 }
 .uisymbols .inlinebuttons input[type=button],
-.uisymbols .inlinebuttons input[type=submit] {
+.uisymbols .inlinebuttons input[type=submit],
+.uisymbols .inlinebuttons a {
  font-size: 100%;
 }
 .inlinebuttons input[type=button]:hover,
-.inlinebuttons input[type=submit]:hover {
+.inlinebuttons input[type=submit]:hover,
+.inlinebuttons a:hover {
  background: #f6f6f6;
  color: #333;
  text-shadow: .1em .1em #fcfcfc;

[Chris.Nelson@…]

Replying to jomae:

Replying to psuter:

Should ${captioned_button('✎', _('Edit'))} be used with the link as well?

That's good to me.

Also, I think the link buttons should have hover and active effects. …

I agree.

[Ryan J Ollos]

I pulled the suggested changes into log:rjollos.git:t6573. For both cases - symbol only and symbol with text - the Edit button needs a bit of work to center it in the row and reduce its size. I haven't found the magic CSS yet.

[Peter Suter]

I added IWikiChangeListener.wiki_page_comment_modified as a Git exercise in log:psuter.git:t6573. (How do I get the ​repo to show up on t.e.o.?)

[Ryan J Ollos]

Replying to psuter:

(How do I get the ​repo to show up on t.e.o.?)

I added psuter.git to the repository index. I assume nothing else needs to be done since [git] cached_repository = false. That is, we don't need to run TracAdmin resync or setup a post-commit hook. Does that sound right?

[Peter Suter]

Replying to rjollos:

I added psuter.git to the repository index.

Thanks!

[Ryan J Ollos]

Replying to psuter:

If you have time for some more style cleanups, check the sources I copied from:

Coding style changes committed in [12942:12944], merged in [12943:12945]. Not sure why I didn't see a functional test failure when running the test locally before commit.

[Ryan J Ollos]

I didn't think through two of the changes from comment:18, which resulted in changes to the message extraction. I'll probably revert those.

[Jun Omae]

Replying to rjollos:

I added psuter.git to the repository index.

I renamed repos:psuter to repos:psuter.hg and added repos:psuter as hidden alias to repos:psuter.hg.

[Peter Suter]

Maybe better to postpone and test this some more.

(Ryan, I hope that doesn't drop comment:19 from your radar. :)

Replying to rjollos:

the Edit button needs a bit of work to center it in the row and reduce its size. I haven't found the magic CSS yet.

Is this for #11130 in general? It already looks acceptable to me here. But I'm no styling expert.

[Ryan J Ollos]

Replying to psuter:

(Ryan, I hope that doesn't drop comment:19 from your radar. :)

Fixed in [13002:13003,13006:13007] (sorry for the noise). I did a make extraction on 1.0-stable at r13002 and there are no new messages. New extraction on trunk in [13005] (the add/delete/modified counts in log message are incorrect since I forgot to look at the changes in tracini.pot).

Replying to rjollos:

the Edit button needs a bit of work to center it in the row and reduce its size. I haven't found the magic CSS yet.

Is this for #11130 in general? It already looks acceptable to me here. But I'm no styling expert.

I think only very minor changes, if any, would be needed. When Use only symbols for buttons is True, the icons seemed a little big, touching the row borders (see below) (from latest changes in log:rjollos.git:t6573). It would be nice to center the buttons a bit more when Use only symbols for buttons is False, but that's a very minor styling change and I wonder if it is even possible since it's an inline element.

[Peter Suter]

Rebased to current trunk and added a unit test in log:psuter.git:t6573-2.

[Jun Omae]

test_edit_comment_of_page_version method sometimes fails.

======================================================================
FAIL: test_edit_comment_of_page_version (trac.wiki.tests.model.WikiPageTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/run/shm/302bd3796163449f419a328fb5ce566b6cc55948/py27-postgres/trac/wiki/tests/model.py", line 287, in test_edit_comment_of_page_version
    """, ('TestPage',)))
AssertionError: Lists differ: [(1, 42, 'joe', '::1', 'Bla bl... != [(2, 43L, u'kate', u'::11', u'...

First differing element 0:
(1, 42, 'joe', '::1', 'Bla bla', 'new comment one', 0)
(2, 43L, u'kate', u'::11', u'Bla', u'edited comment two', 0)

- [(1, 42, 'joe', '::1', 'Bla bla', 'new comment one', 0),
-  (2, 43, 'kate', '::11', 'Bla', 'edited comment two', 0)]
? ^                                                       ^

+ [(2, 43L, u'kate', u'::11', u'Bla', u'edited comment two', 0),
? ^      +  +        +        +       +                        ^

+  (1, 42L, u'joe', u'::1', u'Bla bla', u'new comment one', 0)]

----------------------------------------------------------------------
Ran 1670 tests in 226.622s

FAILED (failures=1)
make: *** [unit-test] Error 1

It seems to be need ORDER BY version.

trac/wiki/tests/model.py

@@ -284 +284 @@
             self.env.db_query("""
                 SELECT version, time, author, ipnr, text, comment, readonly
                 FROM wiki WHERE name=%s
+                ORDER BY version
                 """, ('TestPage',)))
 
         listener = TestWikiChangeListener(self.env)

[Peter Suter]

Oh right, thanks. Committed in [13198].

[Christian Boos]

Nice feature, I just discovered it!

May I suggest some cosmetic improvements?

• display it on the right side of the column (float: right)
• only display it for the currently selected column (mouseover)

trac/htdocs/css/trac.css

@@ -727 +727 @@
 #fieldhist td.version { text-align: center }
 #fieldhist td.comment { width: 100% }
 
+#fieldhist .inlinebuttons {
+ display: none;
+ float: right;
+}
+
 /* Auto-completion interface */
 .suggestions { background: #fff; border: 1px solid #886; color: #222; }
 .suggestions ul {

trac/templates/history_view.html

@@ -19 +19 @@
   <head>
     <title>$title</title>
     <meta name="ROBOTS" content="NOINDEX, NOFOLLOW" />
+    <script type="text/javascript">/*<![CDATA[*/
+      jQuery(function ($) {
+        $("#fieldhist").on("mouseover mouseout", "td", function(event) {
+          $(".inlinebuttons", $(event.target).closest("tr")).toggle();
+        });
+      });
+    /*]]>*/</script>
   </head>
 
   <body>

[Christian Boos]

Replying to rjollos:

I thought the placement of the Edit links was easier to notice with them located next to the comment. Floating them to the right could be okay too, but they might not be as easily noticed.

I think that when you also show/hide them, they get noticed even when located on the far right side.

[Ryan J Ollos]

Changes in comment:26 look good. I have some layout suggestions for the Edit comment page which I'll eventually post in #11792 because they are related to the changes in that ticket.

[Peter Suter]

Should we put a diff view on the edit comment page? (So one can easily see what the comment is / should be about.)

[Christian Boos]

Replying to rjollos:

Changes in comment:26 look good.

Simplified version of the patch committed as r13333.

[Christian Boos]

Replying to psuter:

Should we put a diff view on the edit comment page? (So one can easily see what the comment is / should be about.)

Good idea!

Alternatively, you could consider integrating it somehow in the regular wiki diff view… A similar interface/style could then later be used for editing the changeset comment (#781).

Something like:

[Christian Boos]

In wiki diff view, in place edition of the change comment

[Ryan J Ollos]

Replying to rjollos:

Replying to psuter: I think only very minor changes, if any, would be needed. When Use only symbols for buttons is True, the icons seemed a little big, touching the row borders (see below) (from latest changes in log:rjollos.git:t6573). It would be nice to center the buttons a bit more when Use only symbols for buttons is False, but that's a very minor styling change and I wonder if it is even possible since it's an inline element.

See #11830 for proposed changes.