Skip to main content
Topic: Legacy code. (Read 2759 times) previous topic - next topic
0 Members and 1 Guest are viewing this topic.

Legacy code.

Avatar resizing. Image resizing in general, come to think of it.

Ok, we haz this in Load.php:

Code: [Select]
	// If we're always html resizing, assume it's too large.
if ($modSettings['avatar_action_too_large'] == 'option_html_resize' || $modSettings['avatar_action_too_large'] == 'option_js_resize')
{
$avatar_width = !empty($modSettings['avatar_max_width_external']) ? ' width="' . $modSettings['avatar_max_width_external'] . '"' : '';
$avatar_height = !empty($modSettings['avatar_max_height_external']) ? ' height="' . $modSettings['avatar_max_height_external'] . '"' : '';
}
else
{
$avatar_width = '';
$avatar_height = '';
}

Given that this old resize code doesn't actually reduce cache times or bandwidth usage (because it's just a rendering resize on the fly) and given that I'm throwing in CSS bulletproofness for avatar resize anyway, we can ditch all this crud from Sources. Also, the javascript for the js resize option can go from /scripts, and the options can be dropped from the admin templates.

Which brings me to images in posts. Like the av resize code, they don't cut cache time or bandwidth use either. They also don't allow for different screen res. You just have to pick a good average and hope it works for most users. Same deal: use CSS. Got it running anyway. Works. Bulletproof. No stuff needed in Sources. Images in posts will fit any screen.

Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Legacy code.

Reply #1
Not all of the avatar resize code should go - there is the errant case where you can set a maximum size for external avatars, and it will be downloaded and resized on the server (as, effectively, an uploaded avatar)

But other than that, yes, great plan :)

Re: Legacy code.

Reply #2
Yeah fair enough, although I've never used that option and have no idea how many sites do use it. That's a different code block though innit? (Never looked at where it is stashed).
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Legacy code.

Reply #3
Oh and while I'm looking at browser detection arrays (ugh) I have to wonder how much of this we really need to keep:

Code: [Select]
	/**
* Additional IE checks and settings.
*  - determines the version of the IE browser in use
*  - detects ie4 onward
*  - attempts to distinguish between IE and IE in compatabilty view
*  - checks for old IE on macs as well, since we can
*/
private function _setupIe()
{
$this->_browsers['is_ie_compat_view'] = false;

// get the version of the browser from the msie tag
if (preg_match('~MSIE\s?([0-9][0-9]?.[0-9])~i', $_SERVER['HTTP_USER_AGENT'], $msie_match) === 1)
{
$msie_match[1] = trim($msie_match[1]);
$msie_match[1] = (($msie_match[1] - (int) $msie_match[1]) == 0) ? (int) $msie_match[1] : $msie_match[1];
$this->_browsers['is_ie' . $msie_match[1]] = true;
}

// "modern" ie uses trident 4=ie8, 5=ie9, 6=ie10, even in compatability view
if (preg_match('~Trident/([0-9.])~i', $_SERVER['HTTP_USER_AGENT'], $trident_match) === 1)
{
$this->_browsers['is_ie' . ((int) $trident_match[1] + 4)] = true;

// If trident is set, see the (if any) msie tag in the user agent matches ... if not its in some compatablity view
if (isset($msie_match[1]) && ($msie_match[1] < $trident_match[1] + 4))
$this->_browsers['is_ie_compat_view'] = true;
}

// Detect true IE6 and IE7 and not IE in compat mode.
$this->_browsers['is_ie7'] = !empty($this->_browsers['is_ie7']) && ($this->_browsers['is_ie_compat_view'] === false);
$this->_browsers['is_ie6'] = !empty($this->_browsers['is_ie6']) && ($this->_browsers['is_ie_compat_view'] === false);

// IE mobile 7 or 9, ... shucks why not
if ((!empty($this->_browsers['is_ie7']) && strpos($_SERVER['HTTP_USER_AGENT'], 'IEMobile/7') !== false) || (!empty($this->_browsers['is_ie9']) && strpos($_SERVER['HTTP_USER_AGENT'], 'IEMobile/9') !== false))
{
$this->_browsers['is_ie_mobi'] = true;
$this->_is_mobile = true;
}

// And some throwbacks to a bygone era, deposited here like cholesterol in your arteries
$this->_browsers += array(
'is_ie4' => !empty($this->_browsers['is_ie4']) && !$this->_browsers['is_web_tv'],
'is_mac_ie' => strpos($_SERVER['HTTP_USER_AGENT'], 'MSIE 5.') !== false && strpos($_SERVER['HTTP_USER_AGENT'], 'Mac') !== false
);

// Before IE8 we need to fix IE... lots!
$this->_browsers['ie_standards_fix'] = (($this->_browsers['is_ie6'] === true) || ($this->_browsers['is_ie7'] === true)) ? true : false;

// We may even need a size fix...
$this->_browsers['needs_size_fix'] = (!empty($this->_browsers['is_ie5']) || !empty($this->_browsers['is_ie5.5']) || !empty($this->_browsers['is_ie4'])) && !$this->_browsers['is_mac_ie'];
}

Honestly nobody, but NOBODY, uses IE4 or Mac IE. No themer is EVER going to use code for them in any part of the gui.

We decided to not support IE<8, so not sure if there's any point in keeping the standards_fix. We already have IE version detection in the template anyway, so any required fixes can be done in the CSS like they should be. Can't see the virtue in having a special $context for them.

Size fix is defo a waste of code IMO.

Also, since we are now using the edge mode meta tag for IE in index.template.php, do we give a bugger if the browser is in compatability mode? It'll still render in its highest level, native mode anyway.



By the way, if we're doing mobile detection we should make it a bit more comprehensive.

http://gs.statcounter.com/#mobile_browser-ww-monthly-201211-201301-bar

UC browser is one obvious contender.
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Legacy code.

Reply #4
More stuff. Was just checking Subs.php.

Not sure we need this any more, since subjects don't usually have a huge byte count and the text-overflow attribute can deal with visual shortening without needing any PHP functions.
Code: [Select]
function shorten_subject($subject, $len)
{
    global $smcFunc;

    // It was already short enough!
    if ($smcFunc['strlen']($subject) <= $len)
        return $subject;

    // Shorten it by the length it was too long, and strip off junk from the end.
    return $smcFunc['substr']($subject, 0, $len) . '...';
}

Also, this is the old js resize for avatars, alias crud mentioned in the OP.

Code: [Select]
// Resize avatars the fancy, but non-GD requiring way.
    if ($modSettings['avatar_action_too_large'] == 'option_js_resize' && (!empty($modSettings['avatar_max_width_external']) || !empty($modSettings['avatar_max_height_external'])))
    {
        // @todo Move this over to script.js?
        addInlineJavascript('
        var smf_avatarMaxWidth = ' . (int) $modSettings['avatar_max_width_external'] . ';
        var smf_avatarMaxHeight = ' . (int) $modSettings['avatar_max_height_external'] . ';' . (!isBrowser('ie') ? '
        window.addEventListener("load", smf_avatarResize, false);' : '
        var window_oldAvatarOnload = window.onload;
        window.onload = smf_avatarResize;'));
    }

Plus I don't know where you've gone and hidden it now :P but this stuff (2.0.x example) could be ditched too, since these days it's easy to break words up with CSS.
Code: [Select]
$open_tags = array();
    $message = strtr($message, array("\n" => '<br />'));

    // The non-breaking-space looks a bit different each time.
    $non_breaking_space = $context['utf8'] ? ($context['server']['complex_preg_chars'] ? '\x{A0}' : "\xC2\xA0") : '\xA0';

    // This saves time by doing our break long words checks here.
    if (!empty($modSettings['fixLongWords']) && $modSettings['fixLongWords'] > 5)
    {
        if ($context['browser']['is_gecko'] || $context['browser']['is_konqueror'])
            $breaker = '<span style="margin: 0 -0.5ex 0 0;"> </span>';
        // Opera...
        elseif ($context['browser']['is_opera'])
            $breaker = '<span style="margin: 0 -0.65ex 0 -1px;"> </span>';
        // Internet Explorer...
        else
            $breaker = '<span style="width: 0; margin: 0 -0.6ex 0 -1px;"> </span>';

        // PCRE will not be happy if we don't give it a short.
        $modSettings['fixLongWords'] = (int) min(65535, $modSettings['fixLongWords']);
    }
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Legacy code.

Reply #5
Code: [Select]
// Check if the image is larger than allowed.
                        if (!empty($modSettings['max_image_width']) && !empty($modSettings['max_image_height']))
                        {
                            // For images, we'll want this.
                            require_once($sourcedir . '/Subs-Attachments.php');
                            list ($width, $height) = url_image_size($imgtag);

                            if (!empty($modSettings['max_image_width']) && $width > $modSettings['max_image_width'])
                            {
                                $height = (int) (($modSettings['max_image_width'] * $height) / $width);
                                $width = $modSettings['max_image_width'];
                            }

                            if (!empty($modSettings['max_image_height']) && $height > $modSettings['max_image_height'])
                            {
                                $width = (int) (($modSettings['max_image_height'] * $width) / $height);
                                $height = $modSettings['max_image_height'];
                            }

                            // Set the new image tag.
                            $replaces[$matches[0][$match]] = '[img width=0 height=0]' . $imgtag . '[/img]';
                        }

More old stuff. Can be ditched too.
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Legacy code.

Reply #6
fix_long_words has been removed long ago afair.

shorten_subject is in use, including by SSI.php. Not sure about it. But its existence doesn't prevent implementation of css tricks - if more are necessary (I don't know atm).
The best moment for testing your PR is right after you merge it. Can't miss with that one.

Re: Legacy code.

Reply #7
OK. :)

Was just thinking a bit more about avatars. If you want to keep the option to set maximum avatar sizes in admin settings, which may be good for teh n00bz, you could just alter the existing code to get rid of the js crud and the html height="" and width="" attributes in the img tag, and substitute style="max-height: ***px; max-width: ***px;"

That would give the same scaling behaviour but retain settings for size on the existing page.

I don't think this is worth bothering with for images in posts, but could be good for people who want varying maximum avatar sizes. Some sites like huge ones, others don't.
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Legacy code.

Reply #8
shorten_subject(); still seems useful to me, maybe there will be a reason to shorten a string which isn't displayed inside the templates (thus CSS won't work)..
But I'd rename that function to a simple name like shorten_string();

Am I allowed to rename it?  :D
Thorsten "TE" Eurich
------------------------

Re: Legacy code.

Reply #9
Ah, sounds reasonable to me.

Quote
Am I allowed to rename it?  :D
Are ya asking about rules?...
 TestMonkey blinks.
I've heard the only rule is to break as much as you can and try not to get caught with your hands dirty!1

---
1In particular, the first part of the rule exists because that is Ema-specific implementation.
The best moment for testing your PR is right after you merge it. Can't miss with that one.

Re: Legacy code.

Reply #10
I should add: unless you want to compete for the award!
Bugs creator.
Features destroyer.
Template killer.

Re: Legacy code.

Reply #11
I added a shorten_text function to PBE already ... did not replace shorten_subject with it though (but could).  The main difference is that shorten_text trys to break the string on a word boundary, where as shorten_subject simply counts and cuts.

Shorten subject is used for more than presentation, thats why its still in place
Squish squish. squish, squish, squish.
Find a bug,
Make a wish.

Re: Legacy code.

Reply #12
I added a shorten_text function to PBE already ... did not replace shorten_subject with it though (but could).  The main difference is that shorten_text
Good to know, I'll leave shorten_subject untouched though. IMO the functions should be "merged" at some point.
Thorsten "TE" Eurich
------------------------

Re: Legacy code.

Reply #13
Not sure I see the point of the shorten_text one. Are you using it for things like message index preview pop-ups?

(incidentally, I hate those)
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P