Bitcoin Forum

Other => Meta => Topic started by: PowerGlove on June 10, 2024, 04:02:29 AM



Title: Quoting from locked threads (SMF patch)
Post by: PowerGlove on June 10, 2024, 04:02:29 AM
I've been working on this one for a while now. It was most recently raised (https://bitcointalk.org/index.php?topic=5498796) by garlonicon, but I'm guessing that this issue has been discussed a great many times over the years.

This is something that's handled neatly by the BPIP extension, but there are a lot of people (me included) that prefer not to install extensions, so something built-in would be nice...

I was very tempted to take the same (clever) approach that the BPIP extension took, and build things around the endpoint that SMF's insert-quote feature uses (/index.php?action=quotefast), but, after weighing the pros and cons, I ended up settling on an approach that doesn't introduce new UI elements, feels more SMFish (to me), and avoids depending on JavaScript.

Basically, the approach I took began with a thought that I have to imagine more than a few people have had: Why not just make the existing "quote" button visible in locked threads?

If you go poking around in SMF and make that button appear even in locked threads, you'll quickly discover that it doesn't actually work:

https://talkimg.com/images/2024/06/10/c3Ruv.png

So, the next thing to do is to find a way to (safely) bypass that error: I did that by selectively tacking a ;peek onto the end of the quote button's URL, and then allowing such requests to avoid that lock-check. This turns out to be safe (in terms of not allowing everyone to actually post in locked threads) because (conveniently) there's another lock-check deeper in the code.

Mission accomplished then, right? Well, kind of, but, if the concept is that you're only "peeking" at some post's BBCode, then the "Post reply" page that you land on could definitely use some adjustments:

(*) There's no point in showing the post/preview buttons (in this context), because they'll only error-out if you try to use them (courtesy of that remaining, deeper lock-check).

(*) There's no point in showing things like the old-topic warning, because, as before, you can't actually post anything.

(*) The page title ("Post reply") is now misleading.

(*) The "Topic Summary" at the bottom of the page is unnecessary.

So, I took care of all of that by hiding the things that aren't relevant, and changing the page title to "Inspect message":

https://talkimg.com/images/2024/06/10/c3NPd.png

To help distinguish this different "kind" of quote button (compared to the kind that shows up in not-locked threads), I made the button render at 50% grayscale and added a tooltip to it:

https://talkimg.com/images/2024/06/10/c3Ej5.png

Here are the diffs for @theymos:

Code:
--- baseline/Themes/default/Display.template.php	2010-10-22 01:38:35.000000000 +0000
+++ modified/Themes/default/Display.template.php 2024-06-09 23:32:16.000000000 +0000
@@ -382,30 +382,35 @@
  // If this is the first post, (#0) just say when it was posted - otherwise give the reply #.
  echo '
  <div class="smalltext">&#171; <b>', !empty($message['counter']) ? $txt[146] . ' #' . $message['counter'] : '', ' ', $txt[30], ':</b> ', $message['time'], ' &#187;</div></td>
  <td align="', !$context['right_to_left'] ? 'right' : 'left', '" valign="bottom" height="20" style="font-size: smaller;">';
 
  // Can they reply? Have they turned on quick reply?
  if ($context['can_reply'] && !empty($options['display_quick_reply']))
  echo '
  <a href="', $scripturl, '?action=post;quote=', $message['id'], ';topic=', $context['current_topic'], '.', $context['start'], ';num_replies=', $context['num_replies'], ';sesc=', $context['session_id'], '" onclick="doQuote(', $message['id'], ', \'', $context['session_id'], '\'); return false;">', $reply_button, '</a>';
 
  // So... quick reply is off, but they *can* reply?
  elseif ($context['can_reply'])
  echo '
  <a href="', $scripturl, '?action=post;quote=', $message['id'], ';topic=', $context['current_topic'], '.', $context['start'], ';num_replies=', $context['num_replies'], ';sesc=', $context['session_id'], '">', $reply_button, '</a>';
 
+ // So... they *can't* reply, but if they're logged-in then we should emit a button that at least lets them see/copy the BBCode if needed.
+ elseif (!$context['user']['is_guest'])
+ echo '
+ <a href="', $scripturl, '?action=post;quote=', $message['id'], ';topic=', $context['current_topic'], '.', $context['start'], ';num_replies=', $context['num_replies'], ';sesc=', $context['session_id'], ';peek" style="filter: grayscale(0.5);" title="Inspect message">', $reply_button, '</a>';
+
  // Can the user modify the contents of this post?
  if ($message['can_modify'])
  echo '
  <a href="', $scripturl, '?action=post;msg=', $message['id'], ';topic=', $context['current_topic'], '.', $context['start'], ';sesc=', $context['session_id'], '">', $modify_button, '</a>';
 
  // How about... even... remove it entirely?!
  if ($message['can_remove'])
  echo '
  <a href="', $scripturl, '?action=deletemsg;topic=', $context['current_topic'], '.', $context['start'], ';msg=', $message['id'], ';sesc=', $context['session_id'], '" onclick="return confirm(\'', $txt[154], '?\');">', $remove_button, '</a>';
 
  // What about splitting it off the rest of the topic?
  if ($context['can_split'])
  echo '
  <a href="', $scripturl, '?action=splittopics;topic=', $context['current_topic'], '.0;at=', $message['id'], '">', $split_button, '</a>';
 

Code:
--- baseline/Sources/Post.php	2011-02-07 16:45:09.000000000 +0000
+++ modified/Sources/Post.php 2024-06-09 23:43:55.000000000 +0000
@@ -183,32 +183,36 @@
  $context['can_sticky'] = allowedTo('make_sticky') && !empty($modSettings['enableStickyTopics']);
 
  $context['notify'] = !empty($context['notify']);
  $context['sticky'] = !empty($_REQUEST['sticky']);
  }
 
  // !!! These won't work if you're posting an event!
  $context['can_notify'] = allowedTo('mark_any_notify');
  $context['can_move'] = allowedTo('move_any');
  $context['can_announce'] = allowedTo('announce_topic');
  $context['locked'] = !empty($locked) || !empty($_REQUEST['lock']);
 
  // An array to hold all the attachments for this topic.
  $context['current_attachments'] = array();
 
+ // Is this a "peek" request? (That is, one that's intended to only see/copy the BBCode of a post.)
+ $context['only_peeking'] = !isset($_GET['msg']) && !empty($_GET['quote']) && isset($_GET['peek']);
+
  // Don't allow a post if it's locked and you aren't all powerful.
- if ($locked && !allowedTo('moderate_board'))
+ // But make an exception for "peek" requests (and rely on the similar check in Post2() to pick up the slack).
+ if ($locked && !allowedTo('moderate_board') && !$context['only_peeking'])
  fatal_lang_error(90, false);
 
  // Check the users permissions - is the user allowed to add or post a poll?
  if (isset($_REQUEST['poll']) && $modSettings['pollMode'] == '1')
  {
  // New topic, new poll.
  if (empty($topic))
  isAllowedTo('poll_post');
  // This is an old topic - but it is yours!  Can you add to it?
  elseif ($ID_MEMBER == $ID_MEMBER_POSTER && !allowedTo('poll_add_any'))
  isAllowedTo('poll_add_own');
  // If you're not the owner, can you add to any poll?
  else
  isAllowedTo('poll_add_any');
 
@@ -947,31 +951,31 @@
  $context['error_type'] = 'minor';
  }
 
  // What are you doing?  Posting a poll, modifying, previewing, new post, or reply...
  if (isset($_REQUEST['poll']))
  $context['page_title'] = $txt['smf20'];
  elseif ($context['make_event'])
  $context['page_title'] = $context['event']['id'] == -1 ? $txt['calendar23'] : $txt['calendar20'];
  elseif (isset($_REQUEST['msg']))
  $context['page_title'] = $txt[66];
  elseif (isset($_REQUEST['subject'], $context['preview_subject']))
  $context['page_title'] = $txt[507] . ' - ' . strip_tags($context['preview_subject']);
  elseif (empty($topic))
  $context['page_title'] = $txt[33];
  else
- $context['page_title'] = $txt[25];
+ $context['page_title'] = $context['only_peeking'] ? 'Inspect message' : $txt[25];
 
  // Build the link tree.
  if (empty($topic))
  $context['linktree'][] = array(
  'name' => '<i>' . $txt[33] . '</i>'
  );
  else
  $context['linktree'][] = array(
  'url' => $scripturl . '?topic=' . $topic . '.' . $_REQUEST['start'],
  'name' => $form_subject,
  'extra_before' => '<span' . ($settings['linktree_inline'] ? ' class="smalltext"' : '') . '><b class="nav">' . $context['page_title'] . ' ( </b></span>',
  'extra_after' => '<span' . ($settings['linktree_inline'] ? ' class="smalltext"' : '') . '><b class="nav"> )</b></span>'
  );
 
  // If they've unchecked an attachment, they may still want to attach that many more files, but don't allow more than num_allowed_attachments.
@@ -2027,30 +2031,33 @@
 // Get the topic for display purposes.
 function getTopic()
 {
  global $topic, $db_prefix, $modSettings, $context;
 
  // Calculate the amount of new replies.
  $newReplies = empty($_REQUEST['num_replies']) || $context['num_replies'] <= $_REQUEST['num_replies'] ? 0 : $context['num_replies'] - $_REQUEST['num_replies'];
 
  if (isset($_REQUEST['xml']))
  $limit = "
  LIMIT " . (empty($newReplies) ? '0' : $newReplies);
  else
  $limit = empty($modSettings['topicSummaryPosts']) ? '' : '
  LIMIT ' . (int) $modSettings['topicSummaryPosts'];
 
+ // Previous posts won't be displayed by the template when $context['only_peeking'] is true, so save some cycles...
+ $limit = $context['only_peeking'] ? ' LIMIT 0' : $limit;
+
  // If you're modifying, get only those posts before the current one. (otherwise get all.)
  $request = db_query("
  SELECT IFNULL(mem.realName, m.posterName) AS posterName, m.posterTime, m.body, m.smileysEnabled, m.ID_MSG
  FROM {$db_prefix}messages AS m
  LEFT JOIN {$db_prefix}members AS mem ON (mem.ID_MEMBER = m.ID_MEMBER)
  WHERE m.ID_TOPIC = $topic" . (isset($_REQUEST['msg']) ? "
  AND m.ID_MSG < " . (int) $_REQUEST['msg'] : '') . "
  ORDER BY m.ID_MSG DESC$limit", __FILE__, __LINE__);
  $context['previous_posts'] = array();
  while ($row = mysql_fetch_assoc($request))
  {
  // Censor, BBC, ...
  censorText($row['body']);
  $row['body'] = parse_bbc($row['body'], $row['smileysEnabled'], $row['ID_MSG']);
 

Code:
--- baseline/Themes/default/Post.template.php	2008-04-30 18:30:34.000000000 +0000
+++ modified/Themes/default/Post.template.php 2024-06-09 23:53:13.000000000 +0000
@@ -278,45 +278,49 @@
  echo '
  <input type="hidden" name="eventid" value="', $context['event']['id'], '" />';
 
  // Start the main table.
  echo '
  <table border="0" width="100%" align="center" cellspacing="1" cellpadding="3" class="bordercolor">
  <tr class="titlebg">
  <td>', $context['page_title'], '</td>
  </tr>
  <tr>
  <td class="windowbg">', isset($context['current_topic']) ? '
  <input type="hidden" name="topic" value="' . $context['current_topic'] . '" />' : '', '
  <table border="0" cellpadding="3" width="100%">';
 
  // If an error occurred, explain what happened.
- echo '
+ // Skip this if we're in the context of a "peek" request. (These errors/warnings aren't relevant in this context: the user can't post anything, that's why they're "peeking" in the first place.)
+ if (!$context['only_peeking'])
+ echo '
  <tr', empty($context['post_error']['messages']) ? ' style="display: none"' : '', ' id="errors">
  <td></td>
  <td align="left">
  <div style="padding: 0px; font-weight: bold;', empty($context['error_type']) || $context['error_type'] != 'serious' ? ' display: none;' : '', '" id="error_serious">
  ', $txt['error_while_submitting'], '
  </div>
  <div style="color: red; margin: 1ex 0 2ex 3ex;" id="error_list">
  ', empty($context['post_error']['messages']) ? '' : implode('<br />', $context['post_error']['messages']), '
  </div>
  </td>
  </tr>';
 
  // If it's locked, show a message to warn the replyer.
- echo '
+ // Skip this if we're in the context of a "peek" request. (This warning *seems* germane in this context, but it's not: the whole point of "peek" requests is to allow messages to be inspected from inside locked topics, so warning the user about something that they already know, and against an action that they're unable to take, is pretty silly.)
+ if (!$context['only_peeking'])
+ echo '
  <tr', $context['locked'] ? '' : ' style="display: none"', ' id="lock_warning">
  <td></td>
  <td align="left">
  ', $txt['smf287'], '
  </td>
  </tr>';
 
  // Guests have to put in their name and email...
  if (isset($context['name']) && isset($context['email']))
  {
  echo '
  <tr>
  <td align="right" style="font-weight: bold;', isset($context['post_error']['long_name']) || isset($context['post_error']['no_name']) || isset($context['post_error']['bad_name']) ? 'color: red;' : '', '" id="caption_guestname">
  ', $txt[68], ':
  </td>
@@ -495,30 +499,43 @@
  </tr>
  <tr>
  <td align="right"></td>
  <td class="smalltext">
  <input type="radio" id="poll_hide" name="poll_hide" value="0"', $context['poll_options']['hide'] == 0 ? ' checked="checked"' : '', ' class="check" /> ', $txt['poll_options2'], '<br />
  <input type="radio" id="poll_hide" name="poll_hide" value="1"', $context['poll_options']['hide'] == 1 ? ' checked="checked"' : '', ' class="check" /> ', $txt['poll_options3'], '<br />
  <input type="radio" id="poll_hide" name="poll_hide" value="2"', $context['poll_options']['hide'] == 2 ? ' checked="checked"' : '', empty($context['poll_options']['expire']) ? ' disabled="disabled"' : '', ' class="check" /> ', $txt['poll_options4'], '<br />
  <br />
  </td>
  </tr>';
  }
 
  // The below function prints the BBC, smileys and the message itself out.
  theme_postbox($context['message']);
 
+ // If we're in the context of a "peek" request, avoid emitting what follows past this point... just close off the markup and return.
+ if ($context['only_peeking'])
+ {
+ echo '
+ </table>
+ </td>
+ </tr>
+ </table>
+ </form>';
+
+ return;
+ }
+
  // If this message has been edited in the past - display when it was.
  if (isset($context['last_modified']))
  echo '
  <tr>
  <td valign="top" align="right">
  <b>', $txt[211], ':</b>
  </td>
  <td>
  ', $context['last_modified'], '
  </td>
  </tr>';
 
  // If the admin has enabled the hiding of the additional options - show a link and image for it.
  if (!empty($settings['additional_options_collapsable']))
  echo '

(There are one or two other bits of polish/functionality that I could have pursued, like preventing the template from emitting a few unnecessary background things, and adding the ability to "peek" directly from someone's post/topic history, but, the patch as it stands is kind of in a "sweet spot" complexity-wise, so I'm reluctant to push it further. Also, it's difficult to get theymos to come down from the mountain, so to speak, and when he does, I don't want him put off by a sprawling patch that touches too many files and makes too many changes.)


Title: Re: Quoting from locked threads (SMF patch)
Post by: SamReomo on June 10, 2024, 04:42:28 AM
Another valuable patch by PowerGlove! I never even thought that something like this is possible but when such things are handled by a professional programmer like PowerGlove then surely such things are possible.

I'm very happy to see such feature, although I have tried to quote some locked threads but they aren't working at the moment. I hope soon Theymos may soon implement this amazing feature on the forum's code.


Title: Re: Quoting from locked threads (SMF patch)
Post by: LoyceV on June 10, 2024, 05:41:53 AM
I've ran into not being able to quote so many times, I'd love to see this implemented!


Title: Re: Quoting from locked threads (SMF patch)
Post by: Charles-Tim on June 10, 2024, 06:21:44 AM
It would be good to see this feature on this forum. There is also a lot work to do on this forum if we think about it very well. I do not know why I am just lazy to use the BPIP extension. But I can easily quote locked thread without using it. I only use BPIP.org information which gives enough information that I needed and not need to download anything.


Title: Re: Quoting from locked threads (SMF patch)
Post by: un_rank on June 10, 2024, 07:37:17 AM
Another thorough work you have done here and very much needed considering that none of the alternatives, creating the quote link/date yourself, installing an extension or just posting the link to the post, satisfies every user. I have also never added an extension, so that is not even an option for me.

Theymos, it is that time of the year again... but it has already been more than once in the year  :)

- Jay -


Title: Re: Quoting from locked threads (SMF patch)
Post by: joker_josue on June 10, 2024, 07:45:37 AM
(There are one or two other bits of polish/functionality that I could have pursued, like preventing the template from emitting a few unnecessary background things, and adding the ability to "peek" directly from someone's post/topic history, but, the patch as it stands is kind of in a "sweet spot" complexity-wise, so I'm reluctant to push it further. Also, it's difficult to get theymos to come down from the mountain, so to speak, and when he does, I don't want him put off by a sprawling patch that touches too many files and makes too many changes.)

The simpler the patch, the easier it is to implement and the safer it is. Often the solution is already in the script, just hidden from everyone.

Thank you for this patch @PowerGlove which has all the conditions for a quick implementation.
I think it's something useful, and something we've all wanted to use.


Title: Re: Quoting from locked threads (SMF patch)
Post by: dzungmobile on June 10, 2024, 02:26:26 PM
I do not know why I am just lazy to use the BPIP extension. But I can easily quote locked thread without using it.
You can easily quote posts in locked thread if these posts are purely in texts or simple bolded, italicized. If there are more things in side, colors, customized glow, bbcode, you will struggle to quote them through locked threads.

With complicated posts in locked threads, this SMF patch can help to quoting easily.


Title: Re: Quoting from locked threads (SMF patch)
Post by: Poker Player on June 10, 2024, 02:43:45 PM
This is a very good improvement, until now we had other ways of quoting in locked threads, which were not so convenient, so I think this button was something that was really needed. Congratulations for the good work.


Title: Re: Quoting from locked threads (SMF patch)
Post by: Lafu on June 10, 2024, 03:18:05 PM
I've been working on this one for a while now. It was most recently raised (https://bitcointalk.org/index.php?topic=5498796) by garlonicon,
but I'm guessing that this issue has been discussed a great many times over the years.
Nice work again PowerGlove , and for sure that would be helpful if theymos will be implementing this to the Forum.
Looking forward to have it and im sure that theymos will be doing that.

I've ran into not being able to quote so many times, I'd love to see this implemented!
Same here , and to be honest mostly i am and was to lazy to make it a quoted post.
Copy the quote and adding the source Link was the result for doing it.


Title: Re: Quoting from locked threads (SMF patch)
Post by: hugeblack on June 10, 2024, 03:55:04 PM
There was no problem with the quote, but the problem was always with date, as calculating the time in seconds requires additional effort, so this SMF patch is useful.
+1


Title: Re: Quoting from locked threads (SMF patch)
Post by: dkbit98 on June 10, 2024, 06:18:42 PM
You have my support for this SMAF patch PowerGlove.
BPIP extension is great but it's harder to use it with mobile devices and you don't want to install extensions on Tor browser for desktop.
My only question is what will happen with people who are still using BPIP extension after this patch gets adopted by theymos?
I hope this patch won't break functionality of BPIP extension.


Title: Re: Quoting from locked threads (SMF patch)
Post by: theymos on June 10, 2024, 10:59:09 PM
Nice! It's a pretty self-contained patch, so I was able to add it fairly easily. I left a warning on the post page, though, since I don't think it was otherwise obvious enough what was going on.


Title: Re: Quoting from locked threads (SMF patch)
Post by: SquirrelJulietGarden on June 11, 2024, 04:26:48 AM
I left a warning on the post page, though, since I don't think it was otherwise obvious enough what was going on.
When I click on Quote in a locked thread, I see this warning.
Code:
This topic is locked, so you can't actually reply! 
With your worry on this warning is not obvious enough, that I agree with you, why don't consider to add color for this warning text?

Some colors for consideration like red as it is used for other warnings, when making posts in inactive thread last 120 days; or other colors like brown or purple like you would prefer to use in your post when you highlighted something.

If you don't like red, purple or brown is good color to use for this warning text.


Title: Re: Quoting from locked threads (SMF patch)
Post by: LoyceV on June 11, 2024, 06:39:41 AM
With your worry on this warning is not obvious enough, that I agree with you
Theymos' concern was for the situation without the warning.

The current message is indeed not very obvious, but the missing Post button is a good indicator to look further.


Title: Re: Quoting from locked threads (SMF patch)
Post by: joker_josue on June 11, 2024, 06:52:09 AM
Nice! It's a pretty self-contained patch, so I was able to add it fairly easily. I left a warning on the post page, though, since I don't think it was otherwise obvious enough what was going on.

Thanks @theymos for this quick implementation. I believe it will be useful for everyone, in some moments when we want to quote something from a blocked topic.

Congratulations once again @PowerGlove on your patch being added.  ;)



With your worry on this warning is not obvious enough, that I agree with you
Theymos' concern was for the situation without the warning.

The current message is indeed not very obvious, but the missing Post button is a good indicator to look further.

What @theymos meant is that this implementation is not obvious and we would all see it soon.
Not everyone would quote a post in a closed topic right away, or do it every day. So without a warning, it would be difficult for most of us to realize that this patch had been added.



Title: Re: Quoting from locked threads (SMF patch)
Post by: Charles-Tim on June 11, 2024, 08:25:32 AM
Thanks Theymos for being fast about this. Thanks PowerGlove, you are indeed an asset on this forum. I am hoping you get the award next as you deserve it.

I like the idea that the warning could have been more noticeable if it is in another colour that is not black. Although, even if there is no any warning, it is still good. So leaving it in black is not bad either.

So without a warning, it would be difficult for most of us to realize that this patch had been added.
Before you can see the warning, you would have clicked on the lock thread to see it. If you click on the locked thread, you will see the quote icon. The quote icon was not there before beside merit icon but it is there now which is enough.

The quote icon edges is in faded blue colour which is different from the usual blue colour.


Title: Re: Quoting from locked threads (SMF patch)
Post by: shahzadafzal on June 11, 2024, 04:59:46 PM
Nice! It's a pretty self-contained patch, so I was able to add it fairly easily. I left a warning on the post page, though, since I don't think it was otherwise obvious enough what was going on.

Yet another much-needed patch goes live in no time. Thank you, theymos, and once again, credit goes to PowerGlove, the patch master.

Previously, I had to follow four or five steps using this URL (https://bitcointalk.org/index.php?action=quotefast;quote=48896084;sesc=sesionid;xml) and manually add the message ID and session ID, then copy and paste the quote from the XML. However, this process was not possible using mobile phones.

But now, it's just one click. Thank you, PowerGlove, once again.


https://talkimg.com/images/2024/06/11/cVmeo.png


Title: Re: Quoting from locked threads (SMF patch)
Post by: joker_josue on June 11, 2024, 07:15:24 PM
So without a warning, it would be difficult for most of us to realize that this patch had been added.
Before you can see the warning, you would have clicked on the lock thread to see it. If you click on the locked thread, you will see the quote icon. The quote icon was not there before beside merit icon but it is there now which is enough.

The quote icon edges is in faded blue colour which is different from the usual blue colour.

How many closed topics have you visited today? I haven't visited any today...  :P

If there was no warning, you would only notice this when you enter a closed topic and stay there long enough to notice that button is available.

Unless you are a user very used to visiting a closed topic, it would take some time to notice the difference. We are used to seeing this button on all posts, and even though it has slight graphical differences, you wouldn't notice it the first time you pass by it, as you are used to seeing this button. Therefore, it makes sense that this alert/information was made.


Title: Re: Quoting from locked threads (SMF patch)
Post by: Sandra_hakeem on June 11, 2024, 09:00:58 PM
Nice! It's a pretty self-contained patch, so I was able to add it fairly easily. I left a warning on the post page, though, since I don't think it was otherwise obvious enough what was going on.
it's a good thing that you implemented it without hesitation. To be honest, I haven't faced any challenges trying to quote from locked threads -- maybe that's because I haven't actually tried to.
For the warnings, I'd say - you didn't even have to bother yourself that much; a biggass padlock emoji on a thread, with the reply option missing is enough to tell me that I can only read through i.e if it's one of the old, intriguing sagas of the forum before/during lauda's epoch.
Unless you are a user very used to visiting a closed topic, it would take some time to notice the difference. We are used to seeing this button on all posts, and even though it has slight graphical differences, you wouldn't notice it the first time you pass by it, as you are used to seeing this button. Therefore, it makes sense that this alert/information was made.
This is also very true. The above point wasn't exactly like Charles' -- That he shouldn't have.
Bravo, Gloves!


Title: Re: Quoting from locked threads (SMF patch)
Post by: KingsDen on June 11, 2024, 09:32:20 PM
I've ran into not being able to quote so many times, I'd love to see this implemented!
So many times this happened to me. I solve the problem by just copying the text and insert the author's name in the quote, which definitely doesn't link to anywhere.

Nice! It's a pretty self-contained patch, so I was able to add it fairly easily. I left a warning on the post page, though, since I don't think it was otherwise obvious enough what was going on.
I wasn't sure if this means the patch has been completely incorporated. So, I quickly went to the reputation board which has the highest number of locked threads in the first page and was able to quote a locked thread.
See sample below and sorry for off topic quote.
Here's the problem: you're wasting your time advertising your... thing... in this section of the forum. You should stick to the Gambling board and maybe Services. So now you have like 5 threads in Reputation, all of which haven't gone in your favor. Can I ask what help you think you're doing yourself by putting on such a "show" for all to bear witness? If its for the sake of marketing, I'd have a hard time believing its working.
Congratulations PowerGlove and thanks theymos for prompt implementation.


Title: Re: Quoting from locked threads (SMF patch)
Post by: dkbit98 on June 12, 2024, 10:26:26 AM
That was one of the quickest ever SMF patch adoption by theymos, but like I suspected now BPIP quote copy option is missing when extension is used. and that was probably faster way of quoting posts.
Maybe suchmoon can make small tweaks to make it work again.


Title: Re: Quoting from locked threads (SMF patch)
Post by: PowerGlove on June 12, 2024, 04:16:04 PM
(...) but like I suspected now BPIP quote copy option is missing when extension is used.
Hmm... that's a pity. (I've sent suchmoon a PM offering my help to fix it.)



Nice! It's a pretty self-contained patch, so I was able to add it fairly easily. I left a warning on the post page, though, since I don't think it was otherwise obvious enough what was going on.
I think the warning you added was a good call. Thanks for merging this one! (That was quick. Of course, it had to be in the topic where I said "it's difficult to get theymos to come down from the mountain" that you merged it within a few hours of logging in.) :D



Thanks for the kind words everyone! I'm always tempted to hit that +Merit button on every post that makes me smile, but I (mostly) always succeed in stopping myself: It just seems very self-serving to "encourage" posts like that, but you should all be aware that I appreciate reading them, and that they do mean a lot to me.


Title: Re: Quoting from locked threads (SMF patch)
Post by: Upgrade00 on June 12, 2024, 04:56:39 PM
I'm always tempted to hit that +Merit button on every post that makes me smile, but I (mostly) always succeed in stopping myself: It just seems very self-serving to "encourage" posts like that, but you should all be aware that I appreciate reading them, and that they do mean a lot to me.
Maybe we'll create a patch that stops you from stopping yourself. ;D
But you're the one who needs the appreciation and merits. I'm sure theymos wonders where you have been all his bitcointalk life.

This is and there excellent improvement to the forum and adding a warning message was a great call, otherwise users who find their way to a locked thread without knowing what went on here will be very confused. It shouldn't be long now before you return with another fix.


Title: Re: Quoting from locked threads (SMF patch)
Post by: dkbit98 on June 12, 2024, 05:09:55 PM
Hmm... that's a pity. (I've sent suchmoon a PM offering my help to fix it.)
Thanks!
I created another post in BPIP topic about this issue, but it's nothing urgent if suchmoon is busy.

Now I am wondering what is your upcoming SMF patch idea, the way you are going I think this is slowly becoming addiction for you to patch bitcointalk ;)


Title: Re: Quoting from locked threads (SMF patch)
Post by: Quickseller on June 15, 2024, 07:08:31 AM

Code:
 
+ // Previous posts won't be displayed by the template when $context['only_peeking'] is true, so save some cycles...
+ $limit = $context['only_peeking'] ? ' LIMIT 0' : $limit;
+
  // If you're modifying, get only those posts before the current one. (otherwise get all.)
  $request = db_query("
  SELECT IFNULL(mem.realName, m.posterName) AS posterName, m.posterTime, m.body, m.smileysEnabled, m.ID_MSG
  FROM {$db_prefix}messages AS m
  LEFT JOIN {$db_prefix}members AS mem ON (mem.ID_MEMBER = m.ID_MEMBER)
  WHERE m.ID_TOPIC = $topic" . (isset($_REQUEST['msg']) ? "
  AND m.ID_MSG < " . (int) $_REQUEST['msg'] : '') . "
  ORDER BY m.ID_MSG DESC$limit", __FILE__, __LINE__);
  $context['previous_posts'] = array();
  while ($row = mysql_fetch_assoc($request))
  {
  // Censor, BBC, ...
  censorText($row['body']);
  $row['body'] = parse_bbc($row['body'], $row['smileysEnabled'], $row['ID_MSG']);
 

I am not 100% certain, but this may be an unnecessary query to the database. When quoting from a locked thread, the limit is set to zero, so no rows will be returned in the query, but the query will still run.


Title: Re: Quoting from locked threads (SMF patch)
Post by: PowerGlove on June 15, 2024, 10:05:02 AM
I am not 100% certain, but this may be an unnecessary query to the database. When quoting from a locked thread, the limit is set to zero, so no rows will be returned in the query, but the query will still run.
Yup, the query will run, but the intention there is for the query optimizer to recognize the LIMIT 0 and internally reduce that query down to something that quickly returns an empty set (look for "LIMIT Query Optimization" in the MySQL documentation).

In general, when you spot an odd-looking construct in one of my SMF patches, it's likely because I estimated that the alternative way(s) of doing that thing would have led to a less compact diff. Obviously, I'm not going to favor small diffs over every other concern, but it is something that heavily factors into my decision-making (because, in turn, I believe it's something that factors into theymos' decision-making).


Title: Re: Quoting from locked threads (SMF patch)
Post by: theymos on June 16, 2024, 03:58:18 PM
I am not 100% certain, but this may be an unnecessary query to the database. When quoting from a locked thread, the limit is set to zero, so no rows will be returned in the query, but the query will still run.

You're right that a maximally-performant implementation would definitely omit the query entirely. Even a query like "SELECT 1;" is slower than just about anything done within PHP, since a network request introduces a (relative) ton of latency.

However, I would consider it a slightly worse patch if it fixed this because it would increase the code complexity a fair bit, and the only benefit would be saving a few milliseconds on a page that will probably only be accessed a handful of times each day. In fact, if I'd been writing this, I probably would've left the query as-is, since that would've only used a couple of additional miliseconds.


Title: Re: Quoting from locked threads (SMF patch)
Post by: PowerGlove on June 16, 2024, 09:59:46 PM
In fact, if I'd been writing this, I probably would've left the query as-is, since that would've only used a couple of additional miliseconds.
I nearly left it as-is, but that particular spot (just above the query and just below the stuff already establishing a $limit variable) jumped out at me: it's just such a neat/tempting location to place a little optimization like that...

But, thinking about it now and in terms of profiling the code over a day instead of only in request-specific isolation, it could have been a mistake for me to add it: I mean, the additional bytecode generated for testing $context['only_peeking'] and then branching is negligible, but it is present during every invocation of getTopic() (even the invocations that have nothing to do with quoting from locked threads), so depending on the ratio between ;peek requests and ordinary ones, a complete tally of time-saved vs. time-spent might not be as clear-cut as I guessed.


Title: Re: Quoting from locked threads (SMF patch)
Post by: Quickseller on June 16, 2024, 11:18:51 PM
I am not 100% certain, but this may be an unnecessary query to the database. When quoting from a locked thread, the limit is set to zero, so no rows will be returned in the query, but the query will still run.
Yup, the query will run, but the intention there is for the query optimizer to recognize the LIMIT 0 and internally reduce that query down to something that quickly returns an empty set (look for "LIMIT Query Optimization" in the MySQL documentation).

In general, when you spot an odd-looking construct in one of my SMF patches, it's likely because I estimated that the alternative way(s) of doing that thing would have led to a less compact diff. Obviously, I'm not going to favor small diffs over every other concern, but it is something that heavily factors into my decision-making (because, in turn, I believe it's something that factors into theymos' decision-making).
Yes, when such a query is executed, there shouldn't be any rows that are parsed, but some database resources will still be consumed.

I certainly agree that the particular change does prioritize code simplicity, which is fair.

I am not 100% certain, but this may be an unnecessary query to the database. When quoting from a locked thread, the limit is set to zero, so no rows will be returned in the query, but the query will still run.

You're right that a maximally-performant implementation would definitely omit the query entirely. Even a query like "SELECT 1;" is slower than just about anything done within PHP, since a network request introduces a (relative) ton of latency.

However, I would consider it a slightly worse patch if it fixed this because it would increase the code complexity a fair bit, and the only benefit would be saving a few milliseconds on a page that will probably only be accessed a handful of times each day. In fact, if I'd been writing this, I probably would've left the query as-is, since that would've only used a couple of additional miliseconds.
I wouldn't necessarily be worried about the user experience of a single request (which, as you say, will be negligible), but of the additional resources consumed. This is coming from a perspective of not knowing where the resource bottlenecks are if any exist.

It seems that PowerGlove is aware that you prioritize simple changes, which is a fair priority, as a more complex change can be more difficult to test, and has the potential to break things and/or introduce a vulnerability.

I am not an expert on the SMF code base, but if there is a bottleneck in the database, the query could be eliminated with the following change to the getTopic topic in Post.php.

Code:
function getTopic()
{
    global $topic, $db_prefix, $modSettings, $context;

    // If we're only peeking, we don't need to fetch previous posts
    if ($context['only_peeking'])
    {
        $context['previous_posts'] = array();
        return;
    }

    // Calculate the amount of new replies.
    $newReplies = empty($_REQUEST['num_replies']) || $context['num_replies'] <= $_REQUEST['num_replies'] ? 0 : $context['num_replies'] - $_REQUEST['num_replies'];

    if (isset($_REQUEST['xml']))
        $limit = "
        LIMIT " . (empty($newReplies) ? '0' : $newReplies);
    else
        $limit = empty($modSettings['topicSummaryPosts']) ? '' : '
        LIMIT ' . (int) $modSettings['topicSummaryPosts'];

    // If you're modifying, get only those posts before the current one. (otherwise get all.)
    $request = db_query("
        SELECT IFNULL(mem.realName, m.posterName) AS posterName, m.posterTime, m.body, m.smileysEnabled, m.ID_MSG
        FROM {$db_prefix}messages AS m
            LEFT JOIN {$db_prefix}members AS mem ON (mem.ID_MEMBER = m.ID_MEMBER)
        WHERE m.ID_TOPIC = $topic" . (isset($_REQUEST['msg']) ? "
            AND m.ID_MSG < " . (int) $_REQUEST['msg'] : '') . "
        ORDER BY m.ID_MSG DESC$limit", __FILE__, __LINE__);
    $context['previous_posts'] = array();
    while ($row = mysql_fetch_assoc($request))
    {
        // Censor, BBC, ...
        censorText($row['body']);
        $row['body'] = parse_bbc($row['body'], $row['smileysEnabled'], $row['ID_MSG']);

        $context['previous_posts'][] = $row;
    }
    mysql_free_result($request);
}


Title: Re: Quoting from locked threads (SMF patch)
Post by: suchmoon on June 17, 2024, 12:07:33 AM
But, thinking about it now and in terms of profiling the code over a day instead of only in request-specific isolation, it could have been a mistake for me to add it: I mean, the additional bytecode generated for testing $context['only_peeking'] and then branching is negligible, but it is present during every invocation of getTopic() (even the invocations that have nothing to do with quoting from locked threads), so depending on the ratio between ;peek requests and ordinary ones, a complete tally of time-saved vs. time-spent might not be as clear-cut as I guessed.

Maybe you could eliminate the call to getTopic altogether? Still branching as many times however the cost would likely be offset by not having the function call.

Not that it matters much, this seems such a minor issue.


Title: Re: Quoting from locked threads (SMF patch)
Post by: PowerGlove on June 17, 2024, 12:21:44 AM
Not that it matters much, this seems such a minor issue.
It's an incredibly minor issue, and given the unnecessary discussion it's caused, I regret including the optimization to begin with. :-\