Edgewall Software
Modify

Opened 11 years ago

Closed 4 years ago

Last modified 4 years ago

#6573 closed enhancement (fixed)

Allow change comment edit after commit.

Reported by: willow Owned by: Peter Suter
Priority: lowest Milestone: 1.1.3
Component: wiki system Version: 0.10.4
Severity: minor Keywords: comment
Cc: Thijs Triemstra, Ryan J Ollos
Release Notes:

Wiki page version comments in change history can now be edited. (Requires WIKI_ADMIN permission.)

API Changes:

Added new optional method wiki_page_comment_modified() in IWikiChangeListener.

Description

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.

Attachments (4)

T6573-EditWikiPageVersionComments.diff (6.4 KB ) - added by Peter Suter 4 years ago.
WikiStartHistory.png (34.5 KB ) - added by Ryan J Ollos 4 years ago.
Icons.png (15.8 KB ) - added by Ryan J Ollos 4 years ago.
t6573-edit-comment-in-diff-view.png (78.8 KB ) - added by Christian Boos 4 years ago.
In wiki diff view, in place edition of the change comment

Download all attachments as: .zip

Change History (36)

comment:1 Changed 11 years ago by Emmanuel Blot

Cc: consider added

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

comment:2 Changed 10 years ago by Christian Boos

Cc: consider removed
Keywords: comment added
Milestone: 0.10.51.0

Related to #454.

comment:3 Changed 8 years ago by Christian Boos

Milestone: 1.0unscheduled

Milestone 1.0 deleted

comment:4 Changed 8 years ago by Christian Boos

Milestone: triagingnext-major-0.1X
Priority: normallowest

Metadata editing.

comment:5 Changed 7 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

comment:6 Changed 5 years ago by john.ferg@…

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

Changed 4 years ago by Peter Suter

comment:7 Changed 4 years ago by 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?

Changed 4 years ago by Ryan J Ollos

Attachment: WikiStartHistory.png added

comment:8 in reply to:  7 ; Changed 4 years ago by 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.

comment:9 Changed 4 years ago by Ryan J Ollos

Cc: Ryan J Ollos added

comment:10 in reply to:  8 ; Changed 4 years ago by 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:

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?

Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:11 in reply to:  10 ; Changed 4 years ago by 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

    diff --git a/trac/htdocs/css/trac.css b/trac/htdocs/css/trac.css
    index e08799d..206425a 100644
    a b input[type=button]:hover, input[type=submit]:hover, input[type=reset]:hover {  
    109109 text-shadow: .1em .1em #fcfcfc;
    110110}
    111111input[type=button]:active, input[type=submit]:active,
    112 input[type=reset]:active {
     112input[type=reset]:active, .inlinebuttons a:active {
    113113 position: relative;
    114114 top: .1em;
    115115 left: .1em;
    label.disabled { color: #d7d7d7 }  
    171171 cursor: pointer;
    172172}
    173173.uisymbols .inlinebuttons input[type=button],
    174 .uisymbols .inlinebuttons input[type=submit] {
     174.uisymbols .inlinebuttons input[type=submit],
     175.uisymbols .inlinebuttons a {
    175176 font-size: 100%;
    176177}
    177178.inlinebuttons input[type=button]:hover,
    178 .inlinebuttons input[type=submit]:hover {
     179.inlinebuttons input[type=submit]:hover,
     180.inlinebuttons a:hover {
    179181 background: #f6f6f6;
    180182 color: #333;
    181183 text-shadow: .1em .1em #fcfcfc;

comment:12 in reply to:  11 Changed 4 years ago by 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.

comment:13 Changed 4 years ago by Peter Suter

Milestone: next-major-releases1.1.2
Owner: changed from Christian Boos to Peter Suter
Status: newassigned

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

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

comment:16 in reply to:  15 ; Changed 4 years ago by 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?

comment:17 in reply to:  16 Changed 4 years ago by Peter Suter

Replying to rjollos:

I added psuter.git to the repository index.

Thanks!

comment:18 in reply to:  10 Changed 4 years ago by 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.

Last edited 4 years ago by Ryan J Ollos (previous) (diff)

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

comment:20 in reply to:  16 Changed 4 years ago by 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.

comment:21 in reply to:  14 ; Changed 4 years ago by Peter Suter

Milestone: 1.1.21.1.3

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.

Changed 4 years ago by Ryan J Ollos

Attachment: Icons.png added

comment:22 in reply to:  21 ; Changed 4 years ago by 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.

Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:23 Changed 4 years ago by Peter Suter

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

comment:24 Changed 4 years ago by 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

    diff --git a/trac/wiki/tests/model.py b/trac/wiki/tests/model.py
    index de44a4b..6ac7f07 100644
    a b class WikiPageTestCase(unittest.TestCase):  
    284284            self.env.db_query("""
    285285                SELECT version, time, author, ipnr, text, comment, readonly
    286286                FROM wiki WHERE name=%s
     287                ORDER BY version
    287288                """, ('TestPage',)))
    288289
    289290        listener = TestWikiChangeListener(self.env)

comment:25 Changed 4 years ago by Peter Suter

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Oh right, thanks. Committed in [13198].

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

    diff --git a/trac/htdocs/css/trac.css b/trac/htdocs/css/trac.css
    index b91fba7..e712dd3 100644
    a b table.listing pre { white-space: pre-wrap }  
    727727#fieldhist td.version { text-align: center }
    728728#fieldhist td.comment { width: 100% }
    729729
     730#fieldhist .inlinebuttons {
     731 display: none;
     732 float: right;
     733}
     734
    730735/* Auto-completion interface */
    731736.suggestions { background: #fff; border: 1px solid #886; color: #222; }
    732737.suggestions ul {
  • trac/templates/history_view.html

    diff --git a/trac/templates/history_view.html b/trac/templates/history_view.html
    index f5c5846..9f56921 100644
    a b  
    1919  <head>
    2020    <title>$title</title>
    2121    <meta name="ROBOTS" content="NOINDEX, NOFOLLOW" />
     22    <script type="text/javascript">/*<![CDATA[*/
     23      jQuery(function ($) {
     24        $("#fieldhist").on("mouseover mouseout", "td", function(event) {
     25          $(".inlinebuttons", $(event.target).closest("tr")).toggle();
     26        });
     27      });
     28    /*]]>*/</script>
    2229  </head>
    2330
    2431  <body>

comment:27 in reply to:  8 Changed 4 years ago by 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.

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

Last edited 4 years ago by Ryan J Ollos (previous) (diff)

comment:29 Changed 4 years ago by 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.)

comment:30 in reply to:  28 Changed 4 years ago by Christian Boos

Replying to rjollos:

Changes in comment:26 look good.

Simplified version of the patch committed as r13333.

comment:31 in reply to:  29 Changed 4 years ago by 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:

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

Changed 4 years ago by Christian Boos

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

comment:32 in reply to:  22 Changed 4 years ago by 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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Peter Suter.
The resolution will be deleted.
to The owner will be changed from Peter Suter to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.