On Jan 24, 2008 10:35 AM, huji@svn.wikimedia.org wrote:
// Entry from drop down menu + additional comment$reason .= ': ' . $this->DeleteReason;
Should the ': ' string be localized (below as well as here)? Also, as far as I can tell this will result in summaries like:
Deleted old revision $1: Boilerplate reason: Custom reason
Having two colons is a bit odd.
$mDeletereasonother = Xml::label( wfMsg( 'filedelete-otherreason' ), 'wpReason' );$mDeletereasonotherlist = wfMsgHtml( 'filedelete-reason-otherlist' );$scDeleteReasonList = wfMsgForContent( 'filedelete-reason-dropdown' );$mDeleteReasonList = '';$delcom = Xml::label( wfMsg( 'filedelete-comment' ), 'wpDeleteReasonList' );
It seems incredibly confusing to use local variables whose names begin with $m. An initial lowercase 'm' prefix is used to indicate member variables. Either these should be made member variables, or the 'm' should be dropped. You also have variables that are named identically except for the 'm' ($deleteReasonList vs. $mDeleteReasonList), which is even more confusing.
filedelete-comment and filedelete-otherreason seem to allow arbitrary HTML.
$value = trim( htmlspecialchars($option) );
Consider not escaping ampersands here, so that entities can be used -- really we only want to ban tags.
} elseif ( substr( $value, 0, 1) == '*' && substr( $value, 1, 1) != '*' ) {// A new group is starting ...$value = trim( substr( $value, 1 ) );$deleteReasonList .= "$optgroup<optgroup label=\"$value\">";$optgroup = "</optgroup>";} elseif ( substr( $value, 0, 2) == '**' ) {
It would probably be simpler to read if you reversed these two elseifs. Then you could drop the second part of the (current) first one's condition, and just check substr( $value, 0, 1 ) == '*'.
Also, maybe a clearer name for $optgroup? Like $close or $closeoptgroup or something? The current one is okay, I guess.
if ( $mDeleteReasonList === $value)$selected = ' selected="selected"';
Indentation is wrong here. The second line should be indented by one more tab.
Thanks for pointing them out. I'm going to update this and other reason dropdowns, so they would use a function to generate their dropdown rather a similar code pasted in different places. I will also make the code cleaner, as you mentioned.
Cheers,
Huji
On 1/25/08, Simetrical Simetrical+wikilist@gmail.com wrote:
On Jan 24, 2008 10:35 AM, huji@svn.wikimedia.org wrote:
// Entry from drop down menu +additional comment
$reason .= ': ' . $this->DeleteReason;Should the ': ' string be localized (below as well as here)? Also, as far as I can tell this will result in summaries like:
Deleted old revision $1: Boilerplate reason: Custom reason
Having two colons is a bit odd.
$mDeletereasonother = Xml::label( wfMsg('filedelete-otherreason' ), 'wpReason' );
$mDeletereasonotherlist = wfMsgHtml('filedelete-reason-otherlist' );
$scDeleteReasonList = wfMsgForContent('filedelete-reason-dropdown' );
$mDeleteReasonList = '';$delcom = Xml::label( wfMsg( 'filedelete-comment' ),'wpDeleteReasonList' );
It seems incredibly confusing to use local variables whose names begin with $m. An initial lowercase 'm' prefix is used to indicate member variables. Either these should be made member variables, or the 'm' should be dropped. You also have variables that are named identically except for the 'm' ($deleteReasonList vs. $mDeleteReasonList), which is even more confusing.
filedelete-comment and filedelete-otherreason seem to allow arbitrary HTML.
$value = trim( htmlspecialchars($option));
Consider not escaping ampersands here, so that entities can be used -- really we only want to ban tags.
} elseif ( substr( $value, 0, 1) == '*'&& substr( $value, 1, 1) != '*' ) {
// A new group is starting ...$value = trim( substr( $value, 1) );
$deleteReasonList .="$optgroup<optgroup label="$value">";
$optgroup = "</optgroup>";} elseif ( substr( $value, 0, 2) == '**') {
It would probably be simpler to read if you reversed these two elseifs. Then you could drop the second part of the (current) first one's condition, and just check substr( $value, 0, 1 ) == '*'.
Also, maybe a clearer name for $optgroup? Like $close or $closeoptgroup or something? The current one is okay, I guess.
if ( $mDeleteReasonList ===$value)
$selected = 'selected="selected"';
Indentation is wrong here. The second line should be indented by one more tab.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org http://lists.wikimedia.org/mailman/listinfo/wikitech-l
I introduced a new XML method in http://svn.wikimedia.org/viewvc/mediawiki?view=rev&revision=30150
Using that, we will get rid of some of the variables you notified, and a unique way will be used for action dropdowns (including page and file deletion, and protection forms).
Before updating those forms, I'd be thankful to have comments about the Xml::reasonDropDown thing
Huji
On 1/25/08, Huji huji.huji@gmail.com wrote:
Thanks for pointing them out. I'm going to update this and other reason dropdowns, so they would use a function to generate their dropdown rather a similar code pasted in different places. I will also make the code cleaner, as you mentioned.
Cheers,
Huji
On 1/25/08, Simetrical Simetrical+wikilist@gmail.com wrote:
On Jan 24, 2008 10:35 AM, huji@svn.wikimedia.org wrote:
// Entry from drop down menu +additional comment
$reason .= ': ' . $this->DeleteReason;Should the ': ' string be localized (below as well as here)? Also, as far as I can tell this will result in summaries like:
Deleted old revision $1: Boilerplate reason: Custom reason
Having two colons is a bit odd.
$mDeletereasonother = Xml::label( wfMsg('filedelete-otherreason' ), 'wpReason' );
$mDeletereasonotherlist = wfMsgHtml('filedelete-reason-otherlist' );
$scDeleteReasonList = wfMsgForContent('filedelete-reason-dropdown' );
$mDeleteReasonList = '';$delcom = Xml::label( wfMsg( 'filedelete-comment' ),'wpDeleteReasonList' );
It seems incredibly confusing to use local variables whose names begin with $m. An initial lowercase 'm' prefix is used to indicate member variables. Either these should be made member variables, or the 'm' should be dropped. You also have variables that are named identically except for the 'm' ($deleteReasonList vs. $mDeleteReasonList), which is even more confusing.
filedelete-comment and filedelete-otherreason seem to allow arbitrary HTML.
$value = trim(htmlspecialchars($option) );
Consider not escaping ampersands here, so that entities can be used -- really we only want to ban tags.
} elseif ( substr( $value, 0, 1) =='*' && substr( $value, 1, 1) != '*' ) {
// A new group is starting ...$value = trim( substr( $value,1 ) );
$deleteReasonList .="$optgroup<optgroup label="$value">";
$optgroup = "</optgroup>";} elseif ( substr( $value, 0, 2) =='**' ) {
It would probably be simpler to read if you reversed these two elseifs. Then you could drop the second part of the (current) first one's condition, and just check substr( $value, 0, 1 ) == '*'.
Also, maybe a clearer name for $optgroup? Like $close or $closeoptgroup or something? The current one is okay, I guess.
if ( $mDeleteReasonList ===$value)
$selected = 'selected="selected"';
Indentation is wrong here. The second line should be indented by one more tab.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org http://lists.wikimedia.org/mailman/listinfo/wikitech-l
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Huji:
H> I introduced a new XML method in H> http://svn.wikimedia.org/viewvc/mediawiki?view=rev&revision=30150
i don't quite see the point of this (although that might be because it's almost entirely undocumented :).
from what i can see, it's meant to create <optgroup>s based on input that resembles a MediaWiki list. there doesn't seem to be anything specific to 'reason'-fields though, so i think a more appropriate name might be something like optGroupSelect.
also, a more code-friendly input format than \n-separated lines might be nice for calling it when your options don't come from a wiki page.
- river.
from what i can see, it's meant to create <optgroup>s based on input
that resembles a MediaWiki list. there doesn't seem to be anything specific to 'reason'-fields though, so i think a more appropriate name might be something like optGroupSelect. <<
Well, this thing is going to replace the dropdown code used on Deletion and Protection forms. Those dropdowns were all based on the same coding idea, but with lots of variables defined etc. The idea behind creating reasonDropDown (which can be renamed, of course) is to provide a "single" funciton, which will generate the dropdown lists used on these forms.
What it does is just what the dropdowns on Deletion and Protection forms do at the moment; almost nothing more (except for a better support of pre-selecting an item in the list). It doesn't necessarily create optgroups. Whether optgroups are created or only <options> are created depends on the contents of $list variable, which in practice, comes from a MediaWiki message (like 'filedelete-reason-dropdown', etc).
Anyways, I'm open to ideas about how to update it. At the moment, it works just as good as the hard-coded dropdowns work. If there is anywhere to go from here to make it better, I'm ready to go.
By the way, about the naming, we currently have "dateSelector" and "namespaceSelector" functions. What do you think of reasonSelector?
also, a more code-friendly input format than \n-separated lines might
be nice for calling it when your options don't come from a wiki page. <<
At the moment, the only places where it can be used are those were the list comes from a MediaWiki message. I can't imagine how options can't come from a wiki page, so can you give an example?
Huji
With r30216 I modified Xml::reasonDropDown (now known as Xml::listDropDown) and used it in the three forms which have a reason dropdown at the moment.
Huji
wikitech-l@lists.wikimedia.org