Last modified: 2008-11-03 00:14:18 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 15399 - Wikibits patch #2 - disable unused code
Wikibits patch #2 - disable unused code
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Parser (Other open bugs)
unspecified
All All
: Normal enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-01 02:14 UTC by SharkD
Modified: 2008-11-03 00:14 UTC (History)
3 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
unified diff file (1.96 KB, patch)
2008-09-01 02:15 UTC, SharkD
Details

Description SharkD 2008-09-01 02:14:31 UTC
Disable unused portions of code. Optionally, keep them in case they're needed later.
Comment 1 SharkD 2008-09-01 02:15:06 UTC
Created attachment 5236 [details]
unified diff file
Comment 2 SharkD 2008-09-01 02:16:01 UTC
I'm having trouble adding attachments. I keep getting an "was unable to add attachment, please use the 'create attachment' link" message.
Comment 3 Daniel Friesen 2008-09-01 08:25:57 UTC
You've just commented out code here and recommended uncommenting if someone wants to support it.

This is bad style, this requires people to modify core code just to enable a feature already inside of core. We make every effort to make sure people can make use of MediaWiki WITHOUT needing to modify code. This code change merely makes it harder for people to update if they want to use a feature inside of core. That alone makes me recommend WONTFIX.

Additionally as I commented on the other bug:
----
I don't like this idea. Just because we don't support it now, doesn't mean that
we won't support it in the future, and that extensions won't make use of it.
Show/Hide even supports it.

Personally, I just started using Drupal for my own portfolio, and I noticed
that drupal has a nice feature with table headers. I don't know if this is JS,
or browser built in. But when you scroll past the top of the table, the header
follows at the top of the page so you always know what a column is about. That
kind of thing makes me feel like finding a way to make it possible to use table
headers inside of MediaWiki and make that feature possible.
Comment 4 SharkD 2008-09-01 15:33:16 UTC
IIRC, the THEAD element is not supported by Wikimedia software in any form, even if specifially added by hand.

The alternate row coloring can be re-enabled by setting the ts_alternate_row_colors parameter to equal 'true'.
Comment 5 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-01 19:15:24 UTC
>-var ts_alternate_row_colors = true;
>+var ts_alternate_row_colors = false;

This is a good change.  It should be possible to re-enable this in MediaWiki:Common.js if necessary, but this should be off by default.  I've been told it can cause noticeable lag for large tables.  If we do want to support such classes, it should be done in the software, not client-side after the page has already been served.

>-	ts_alternate(table);		
>+	if (ts_alternate_row_colors) {
>+		ts_alternate(table);
>+	}

. . . and this is an even better change, the old behavior should qualify as something of a bug.  I've committed these two changes in r40311.

The other part I don't see a need for.  Does it slow anything down?  If not, we may as well leave it in case we need it later.
Comment 6 SharkD 2008-09-01 21:37:46 UTC
I thought of a better idea: why not enable it using a class name? This gives users control over it instead of devs.

However, AFAIK, the code doesn't actually *do* anything. Are there any skins that style the "even" and "odd" classes differently?
Comment 7 SharkD 2008-09-01 21:41:01 UTC
(In reply to comment #5)
> If we do want to support
> such classes, it should be done in the software, not client-side after the page
> has already been served.

Woops. Missed this bit. If you do it server-side, then the coloring will be all wrong when the table gets (re-)sorted.

Otherwise, I've seen people implement alternate row coloring using templates, and I'd like the script to be as fast as possible.
Comment 8 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-02 00:02:34 UTC
(In reply to comment #6)
> I thought of a better idea: why not enable it using a class name? This gives
> users control over it instead of devs.

Not sure if it's practical right now to parse classes server-side . . .

> However, AFAIK, the code doesn't actually *do* anything. Are there any skins
> that style the "even" and "odd" classes differently?

I don't think so, but it's potentially useful functionality.  I wouldn't put a high priority on making it work, though, there are lots of more useful things to do.

(In reply to comment #7)
> Woops. Missed this bit. If you do it server-side, then the coloring will be all
> wrong when the table gets (re-)sorted.

Hmm, right.  The sorting script will still have to recompute them when you sort.  But it shouldn't have to on page load.  It might be okay if it takes an extra second when the user explicitly chooses to push a button, if that's the cost of particular functionality, but it's not okay if it takes an extra second for the page to load.
Comment 9 SharkD 2008-09-02 01:10:40 UTC
(In reply to comment #8)
> (In reply to comment #6)
> > I thought of a better idea: why not enable it using a class name? This gives
> > users control over it instead of devs.
> Not sure if it's practical right now to parse classes server-side . . .

No, no... The class name would be used to activate the script client-side. This is exactly how the "sortable" class works currently. I.e., the "sortable" class is ignored by the server; the javascript is what picks up on this and then makes the tables sortable.

> (In reply to comment #7)
> > Woops. Missed this bit. If you do it server-side, then the coloring will be all
> > wrong when the table gets (re-)sorted.
> Hmm, right.  The sorting script will still have to recompute them when you
> sort.  But it shouldn't have to on page load.  It might be okay if it takes an
> extra second when the user explicitly chooses to push a button, if that's the
> cost of particular functionality, but it's not okay if it takes an extra second
> for the page to load.

Well, I would personally rather wait an extra second for the page to page load than wait a second each a button is clicked. UI latency is a bigger issue than loading times, IMO.
Comment 10 SharkD 2008-09-02 01:33:26 UTC
I forgot to add that there's a variable called SORT_COLUMN_INDEX that isn't being used anywhere as far as I can tell. I think it can be safely removed as well.
Comment 11 SharkD 2008-09-02 01:50:23 UTC
Also, the sort icons themselves have this bit of code attached to them:

onclick="ts_resortTable(this);return false;"

Is the "return false" portion necessary for some reason?
Comment 12 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-02 02:13:03 UTC
(In reply to comment #9)
> No, no... The class name would be used to activate the script client-side.

Only if the addition of the row coloring on initial view is done client-side, which I don't think is acceptable.

> Well, I would personally rather wait an extra second for the page to page load
> than wait a second each a button is clicked. UI latency is a bigger issue than
> loading times, IMO.

It's not either-or.  You'll *always* have to wait when the button is clicked (modulo any optimizations that can be made).  On the other hand, there's no reason you have to *also* wait when the page loads.

As I said, though, I wouldn't worry about it, since there doesn't seem to be much demand for it.

(In reply to comment #10)
> I forgot to add that there's a variable called SORT_COLUMN_INDEX that isn't
> being used anywhere as far as I can tell. I think it can be safely removed as
> well.

Removed this in r40322, thanks.

(In reply to comment #11)
> Also, the sort icons themselves have this bit of code attached to them:
> 
> onclick="ts_resortTable(this);return false;"
> 
> Is the "return false" portion necessary for some reason?

To avoid the icons doing anything else when you click them, I assume, like jumping the screen to the top of the page.
Comment 13 Tim Starling 2008-09-02 02:18:26 UTC
The whole sorted table thing was just copied verbatim from another website.
That's why it has a whole lot of useless irrelevant code. ts_alternate() should
be removed entirely, not disabled. The THEAD stuff is harmless and can be left
in.
Comment 14 SharkD 2008-09-02 05:16:02 UTC
(In reply to comment #13)
> The whole sorted table thing was just copied verbatim from another website.
> That's why it has a whole lot of useless irrelevant code. ts_alternate() should
> be removed entirely, not disabled. The THEAD stuff is harmless and can be left
> in.

I'd rather comment it out. It can always be uncommented when Mediawiki adds support for the THEAD element. As it is now it's just dead weight.
Comment 15 SharkD 2008-09-02 21:31:13 UTC
(In reply to comment #12)
> (In reply to comment #9)
> > Is the "return false" portion necessary for some reason?
> To avoid the icons doing anything else when you click them, I assume, like
> jumping the screen to the top of the page.

Correct. However, this is only necessary due to the icon being surrounded by <a> tags. Why are these tags necessary? If a browser doesn't support the "onclick" attribute of the icon itself, then it would almost certainly choke on some other part of the script.
Comment 16 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-03 00:07:55 UTC
(In reply to comment #14)
> I'd rather comment it out. It can always be uncommented when Mediawiki adds
> support for the THEAD element. As it is now it's just dead weight.

There's no point in commenting it out.  We're using version control here, it's not like we can't retrieve it later if we delete it.

(In reply to comment #15)
> Correct. However, this is only necessary due to the icon being surrounded by
> <a> tags. Why are these tags necessary? If a browser doesn't support the
> "onclick" attribute of the icon itself, then it would almost certainly choke on
> some other part of the script.

Well, it's semantically a link.  This will give correct behavior in terms of the cursor turning into a finger, for instance.  That can be emulated using CSS, but there may be other things associated with a link that can't.  It's best to use the correct semantic markup.
Comment 17 SharkD 2008-09-03 22:46:25 UTC
Could the onclick be removed from the image/span and the href attribute of the anchor changed to href="javascript:do_this_function();return false;"?
Comment 18 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-14 19:04:13 UTC
We don't like using JavaScript URLs, mostly.  This is a good principle if we have a fallback non-JS URL (href can be set to non-JS content, onclick used for JS content), but we don't here, and can't, so I'm not sure why we would want to either use it or avoid it in this case.  Why do you suggest changing the onclick to an href?  It seems like there's no difference.  For the informative display in the lower left of the browser or whatever?
Comment 19 SharkD 2008-09-17 01:34:25 UTC
Moving the function call to the HREF attribute has the result of eliminating the need for the SPAN element. The leftover "sortarrow" class could then be applied to either the anchor or the image.
Comment 20 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-10-08 18:36:02 UTC
Okay, so is there any more unused code that needs to be disabled, or can this bug be marked FIXED?
Comment 21 Siebrand Mazeland 2008-11-03 00:14:18 UTC
Marking fixed per comment 20.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links