r41623 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r41622‎ | r41623 | r41624 >
Date:23:09, 3 October 2008
Author:aaron
Status:old (Comments)
Tags:
Comment:
* Use one big submit button
* Disallow submitting blank comments in the UI
Modified paths:
  • /trunk/extensions/CodeReview/CodeReview.i18n.php (modified) (history)
  • /trunk/extensions/CodeReview/CodeRevisionStatusSetter.php (modified) (history)
  • /trunk/extensions/CodeReview/CodeRevisionTagger.php (modified) (history)
  • /trunk/extensions/CodeReview/CodeRevisionView.php (modified) (history)
  • /trunk/extensions/CodeReview/SpecialCode.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CodeReview/CodeReview.i18n.php
——@@ -46,6 +46,7 @@
4747 'code-status-fixme' => 'fixme',
4848 'code-status-resolved' => 'resolved',
4949 'code-status-ok' => 'ok',
 50+ 'code-rev-submit' => 'Commit changes',
5051
5152 'codereview-reply-link' => 'reply',
5253
Index: trunk/extensions/CodeReview/SpecialCode.php
——@@ -23,32 +23,42 @@
2424 $view = new CodeRevisionListView( $params[0] );
2525 break;
2626 case 2:
 27+ if( $wgRequest->wasPosted() ) {
 28+ # Add any tags
 29+ $crt = new CodeRevisionTagger( $params[0], $params[1] );
 30+ $crt->execute();
 31+ # Set status
 32+ $crs = new CodeRevisionStatusSetter( $params[0], $params[1] );
 33+ $crs->execute();
 34+ }
 35+ # Adds comments and makes output
 36+ $view = new CodeRevisionView( $params[0], $params[1] );
 37+ break;
2738 case 3:
28 - if( is_numeric( $params[1] ) ) {
29 - $view = new CodeRevisionView( $params[0], $params[1] );
30 - break;
31 - }
32 - if( $params[1] == 'tag' ) {
 39+ if( $params[1] === 'tag' ) {
3340 if( empty($params[2]) )
3441 $view = new CodeRevisionTagListView( $params[0] );
3542 else
3643 $view = new CodeRevisionTagView( $params[0], $params[2] );
3744 break;
38 - } elseif( $params[1] == 'author' ) {
 45+ } elseif( $params[1] === 'author' ) {
3946 if( empty($params[2]) )
4047 $view = new CodeRevisionAuthorListView( $params[0] );
4148 else
4249 $view = new CodeRevisionAuthorView( $params[0], $params[2] );
4350 break;
44 - } elseif( $params[1] == 'status' ) {
 51+ } elseif( $params[1] === 'status' ) {
4552 if( empty($params[2]) )
4653 $view = new CodeRevisionStatusListView( $params[0] );
4754 else
4855 $view = new CodeRevisionStatusView( $params[0], $params[2] );
4956 break;
5057 } else {
51 - # Nonsense param, back out
52 - $view = new CodeRevisionListView( $params[0] );
 58+ # Nonsense parameters, back out
 59+ if( empty($params[1]) )
 60+ $view = new CodeRevisionListView( $params[0] );
 61+ else
 62+ $view = new CodeRevisionView( $params[0], $params[1] );
5363 break;
5464 }
5565 case 4:
Index: trunk/extensions/CodeReview/CodeRevisionTagger.php
——@@ -18,10 +18,6 @@
1919 $repo = $this->mRepo->getName();
2020 $rev = $this->mRev->getId();
2121 $special = SpecialPage::getTitleFor( 'Code', "$repo/$rev" );
22 -
23 - $wgOut->redirect( $special->getFullUrl() );
24 - } else {
25 - throw new MWException( 'Attempted to add invalid tag (fixme UI)' );
2622 }
2723 }
2824
Index: trunk/extensions/CodeReview/CodeRevisionStatusSetter.php
——@@ -18,10 +18,6 @@
1919 $repo = $this->mRepo->getName();
2020 $rev = $this->mRev->getId();
2121 $special = SpecialPage::getTitleFor( 'Code', "$repo/$rev" );
22 -
23 - $wgOut->redirect( $special->getFullUrl() );
24 - } else {
25 - throw new MWException( 'Attempted to save invalid status.' );
2622 }
2723 }
2824
Index: trunk/extensions/CodeReview/CodeRevisionView.php
——@@ -4,11 +4,13 @@
55 class CodeRevisionView extends CodeView {
66
77 function __construct( $repoName, $rev, $replyTarget=null ){
 8+ global $wgRequest;
89 parent::__construct();
910 $this->mRepo = CodeRepository::newFromName( $repoName );
1011 $this->mRev = $this->mRepo ? $this->mRepo->getRevision( intval( $rev ) ) : null;
11 - $this->mReplyTarget = $replyTarget;
1212 $this->mPreviewText = false;
 13+ $this->mReplyTarget = $replyTarget ?
 14+ (int)$replyTarget : $wgRequest->getIntOrNull( 'wpParent' );
1315 }
1416
1517 function execute(){
——@@ -34,7 +36,7 @@
3537 $paths .= $this->formatPathLine( $row->cp_path, $row->cp_action );
3638 }
3739 if( $paths ){
38 - $paths = "<ul>\n$paths</ul>";
 40+ $paths = "<ul>\n$paths</ul>\n";
3941 }
4042 $fields = array(
4143 'code-rev-repo' => $repoLink,
——@@ -43,13 +45,16 @@
4446 'code-rev-status' => $this->statusForm(),
4547 'code-rev-message' => $this->formatMessage( $this->mRev->getMessage() ),
4648 'code-rev-paths' => $paths,
47 - 'code-rev-tags' => $this->formatTags(),
 49+ 'code-rev-tags' => $this->tagForm(),
4850 );
49 - $html = '<table class="mw-codereview-meta">';
 51+
 52+ $special = SpecialPage::getTitleFor( 'Code', $this->mRepo->getName().'/'.$this->mRev->getId() );
 53+ $html = Xml::openElement( 'form', array( 'action' => $special->getLocalUrl(), 'method' => 'post' ) );
 54+ $html .= '<table class="mw-codereview-meta">';
5055 foreach( $fields as $label => $data ) {
5156 $html .= "<tr><td>" . wfMsgHtml( $label ) . "</td><td>$data</td></tr>\n";
5257 }
53 - $html .= '</table>';
 58+ $html .= "</table>\n";
5459
5560 $diffHtml = $this->formatDiff();
5661 if( $diffHtml ) {
——@@ -57,10 +62,10 @@
5863 "<h2>" . wfMsgHtml( 'code-rev-diff' ) . "</h2>" .
5964 "<div class='mw-codereview-diff'>" .
6065 $diffHtml .
61 - "</div>";
 66+ "</div>\n";
6267 }
6368 $html .=
64 - '<h2>'. wfMsgHtml( 'code-comments' ) .'</h2>' .
 69+ "<h2>". wfMsgHtml( 'code-comments' ) ."</h2>\n" .
6570 $this->formatComments();
6671
6772 if( $this->mReplyTarget ) {
——@@ -68,8 +73,14 @@
6974 $id = intval( $this->mReplyTarget );
7075 $html .= "<script type=\"$wgJsMimeType\">addOnloadHook(function(){" .
7176 "document.getElementById('wpReplyTo$id').focus();" .
72 - "});</script>";
 77+ "});</script>\n";
7378 }
 79+ $html .= '<div>' .
 80+ Xml::submitButton( wfMsg( 'code-rev-submit' ), array( 'name' => 'wpSave' ) ) .
 81+ ' ' .
 82+ Xml::submitButton( wfMsg( 'code-rev-comment-preview' ), array( 'name' => 'wpPreview' ) ) .
 83+ '</div>' .
 84+ '</form>';
7485
7586 $wgOut->addHtml( $html );
7687 }
——@@ -109,14 +120,14 @@
110121 if( $wgRequest->wasPosted()
111122 && $wgUser->matchEditToken( $wgRequest->getVal( 'wpEditToken' ) ) ) {
112123 // Look for a posting...
113 - $text = $wgRequest->getText( 'wpReply' );
 124+ $text = $wgRequest->getText( "wpReply{$this->mReplyTarget}" );
114125 $parent = $wgRequest->getIntOrNull( 'wpParent' );
115126 $review = $wgRequest->getInt( 'wpReview' );
116127 $isPreview = $wgRequest->getCheck( 'wpPreview' );
117128 if( $isPreview ) {
118129 // Save the text for reference on later comment display...
119130 $this->mPreviewText = $text;
120 - } else {
 131+ } else if( $text ) { // don't save blank comments
121132 $id = $this->mRev->saveComment( $text, $review, $parent );
122133
123134 // Redirect to the just-saved comment; this avoids POST
——@@ -150,19 +161,17 @@
151162 return "<li>$link ($desc)$diff</li>\n";
152163 }
153164
154 - function formatTags() {
 165+ function tagForm() {
155166 global $wgUser;
156 -
157167 $tags = $this->mRev->getTags();
158168 $list = implode( ", ",
159169 array_map(
160170 array( $this, 'formatTag' ),
161 - $tags ) );
162 -
 171+ $tags )
 172+ ) . '&nbsp;';
163173 if( $wgUser->isAllowed( 'codereview-add-tag' ) ) {
164174 $list .= $this->addTagForm();
165175 }
166 -
167176 return $list;
168177 }
169178
——@@ -173,18 +182,10 @@
174183 $rev = $this->mRev->getId();
175184 $special = SpecialPage::getTitleFor( 'Code', "$repo/$rev/set/status" );
176185 return
177 - Xml::openElement( 'form',
178 - array(
179 - 'action' => $special->getLocalUrl(),
180 - 'method' => 'post' ) ) .
181186 Xml::openElement( 'select',
182187 array( 'name' => 'wpStatus' ) ) .
183188 $this->buildStatusList() .
184 - '</select>' .
185 - Xml::hidden( 'wpEditToken', $wgUser->editToken() ) .
186 - '&nbsp;' .
187 - Xml::submitButton( wfMsg( 'code-rev-status-set' ) ) .
188 - '</form>';
 189+ '</select>';
189190 } else {
190191 return htmlspecialchars( $this->statusDesc( $this->mRev->getStatus() ) );
191192 }
——@@ -208,24 +209,13 @@
209210 $repo = $this->mRepo->getName();
210211 $rev = $this->mRev->getId();
211212 $special = SpecialPage::getTitleFor( 'Code', "$repo/$rev/add/tag" );
212 - return
213 - Xml::openElement( 'form',
214 - array(
215 - 'action' => $special->getLocalUrl(),
216 - 'method' => 'post' ) ) .
217 - Xml::input( 'wpTag', '' ) .
218 - Xml::hidden( 'wpEditToken', $wgUser->editToken() ) .
219 - '&nbsp;' .
220 - Xml::submitButton( wfMsg( 'code-rev-tag-add' ) ) .
221 - '</form>';
 213+ return Xml::input( 'wpTag', '' );
222214 }
223215
224216 function formatTag( $tag ) {
225217 global $wgUser;
226 -
227218 $repo = $this->mRepo->getName();
228219 $special = SpecialPage::getTitleFor( 'Code', "$repo/tag/$tag" );
229 -
230220 return $this->mSkin->link( $special, htmlspecialchars( $tag ) );
231221 }
232222
——@@ -249,7 +239,7 @@
250240 }
251241
252242 function formatCommentInline( $comment ) {
253 - if( $comment->id == $this->mReplyTarget ) {
 243+ if( $comment->id === $this->mReplyTarget ) {
254244 return $this->formatComment( $comment,
255245 $this->postCommentForm( $comment->id ) );
256246 } else {
——@@ -331,34 +321,19 @@
332322 }
333323 $repo = $this->mRepo->getName();
334324 $rev = $this->mRev->getId();
335 - if( $parent ) {
336 - $special = SpecialPage::getTitleFor( 'Code', "$repo/$rev/reply/$parent" );
337 - } else {
338 - $special = SpecialPage::getTitleFor( 'Code', "$repo/$rev" );
339 - }
340325 return '<div class="mw-codereview-post-comment">' .
341326 $preview .
342 - Xml::openElement( 'form',
343 - array(
344 - 'action' => $special->getLocalUrl(),
345 - 'method' => 'post' ) ) .
346327 Xml::hidden( 'wpEditToken', $wgUser->editToken() ) .
347328 ($parent ? Xml::hidden( 'wpParent', $parent ) : '') .
348329 '<div>' .
349330 Xml::openElement( 'textarea', array(
350 - 'name' => 'wpReply',
 331+ 'name' => "wpReply{$parent}",
351332 'id' => "wpReplyTo{$parent}",
352333 'cols' => 40,
353334 'rows' => 5 ) ) .
354335 $text .
355336 '</textarea>' .
356337 '</div>' .
357 - '<div>' .
358 - Xml::submitButton( wfMsg( 'code-rev-comment-submit' ), array( 'name' => 'wpSave' ) ) .
359 - ' ' .
360 - Xml::submitButton( wfMsg( 'code-rev-comment-preview' ), array( 'name' => 'wpPreview' ) ) .
361 - '</div>' .
362 - '</form>' .
363338 '</div>';
364339 }
365340 }

Comments

#Comment by Brion VIBBER (talk | contribs)   23:22, 3 October 2008

For basic saves looks good! Some issues...

[4:21pm] brion: AaronSchulz: if i preview a comment, it saves my status and tag updates [4:21pm] brion: i'm not sure if that's good or bad [4:22pm] brion: and clicking a [reply] link will lose my status/tag changes entirely :(

#Comment by Voice of All (talk | contribs)   23:47, 3 October 2008

Fixed

#Comment by Simetrical (talk | contribs)   01:05, 5 October 2008 
-			} else {
+			} else if( $text ) { // don't save blank comments

Won't this stop saving a comment containing exactly the string "0"?

Status & tagging log