Changes to the behavior of the inline image viewer #129

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
@gamefreak
Collaborator

gamefreak commented May 2, 2012

I created a system for preserving the original link (when necessary)
without the trouble of loading entire webpages to mark history.

When necessary, the original link is saved to a property called
hrefOverride. If that is detected during expand creation then
mouseover/mouseout handlers will be attached to swap the href on demand.

This works well but it can cause the link to turn blue on mouseover.
Without marking the history this has been made nearly impossible to
prevent in the name of user privacy.

I also went back to the original link on thumbnails so there is a fallback method for all links.


On a less closely related change, galleries now display per image captions/titles.


The inline image viewer now handles TEXT types for tumblr.

I do not at this time do anything to protect from malicious HTML. (I trust tumblr enough, but this is a general purpose feature)

Consider porting the tweet viewer to run in the image viewer.

gamefreak added some commits May 2, 2012

Attempt to preserve the original urls when following links.
I created a system for preserving the original link (when necessary)
without the trouble of loading entire webpages to mark history.

When necessary, the original link is saved to a property called
hrefOverride. If that is detected during expand creation then
mouseover/mouseout handlers will be attached to swap the href on demand.

This works well but it can cause the link to turn blue on mouseover.
Without marking the history this has been made nearly impossible to
prevent in the name of user privacy.
Implement TEXT expando type and tumblr handling for it
The inline image viewer now handles TEXT types for tumblr.

I do not at this time do anything to protect from malicious HTML. (I
trust tumblr enough, but this is a general purpose feature)

Consider porting the tweet viewer to run in the image viewer.
@honestbleeps

This comment has been minimized.

Show comment
Hide comment
@honestbleeps

honestbleeps May 2, 2012

Owner

quick question as I've given only a cursory review:

you mention leaving the thumbnail as the original URL...

I may be missing something as I didn't read the code super thoroughly,
but if I'm understanding correctly, this would mean:

thumbnail.href != link.href

if that's true, that's a problem that contributes to peoples' "it's
not purpling my links!" complaints, because some people click the
thumbnails and expect the main link to go purple...

if that's not true, I skimmed the code too fast...

On Wed, May 2, 2012 at 1:16 PM, Scott McClaugherty
reply@reply.github.com
wrote:

I created a system for preserving the original link (when necessary)
without the trouble of loading entire webpages to mark history.

When necessary, the original link is saved to a property called
hrefOverride. If that is detected during expand creation then
mouseover/mouseout handlers will be attached to swap the href on demand.

This works well but it can cause the link to turn blue on mouseover.
Without marking the history this has been made nearly impossible to
prevent in the name of user privacy.

I also went back to the original link on thumbnails so there is a fallback method for all links.


On a less closely related change, galleries now display per image captions/titles.

You can merge this Pull Request by running:

 git pull https://github.com/gamefreak/Reddit-Enhancement-Suite showimage-behavior

Or you can view, comment on it, or merge it online at:

 #129

-- Commit Summary --

  • Display per-image titles/captions for galleries.
  • Keep the original link on the thumbnail.
  • Attempt to preserve the original urls when following links.

-- File Changes --

M lib/reddit_enhancement_suite.user.js (149)

-- Patch Links --

 https://github.com/honestbleeps/Reddit-Enhancement-Suite/pull/129.patch
 https://github.com/honestbleeps/Reddit-Enhancement-Suite/pull/129.diff


Reply to this email directly or view it on GitHub:
#129

Owner

honestbleeps commented May 2, 2012

quick question as I've given only a cursory review:

you mention leaving the thumbnail as the original URL...

I may be missing something as I didn't read the code super thoroughly,
but if I'm understanding correctly, this would mean:

thumbnail.href != link.href

if that's true, that's a problem that contributes to peoples' "it's
not purpling my links!" complaints, because some people click the
thumbnails and expect the main link to go purple...

if that's not true, I skimmed the code too fast...

On Wed, May 2, 2012 at 1:16 PM, Scott McClaugherty
reply@reply.github.com
wrote:

I created a system for preserving the original link (when necessary)
without the trouble of loading entire webpages to mark history.

When necessary, the original link is saved to a property called
hrefOverride. If that is detected during expand creation then
mouseover/mouseout handlers will be attached to swap the href on demand.

This works well but it can cause the link to turn blue on mouseover.
Without marking the history this has been made nearly impossible to
prevent in the name of user privacy.

I also went back to the original link on thumbnails so there is a fallback method for all links.


On a less closely related change, galleries now display per image captions/titles.

You can merge this Pull Request by running:

 git pull https://github.com/gamefreak/Reddit-Enhancement-Suite showimage-behavior

Or you can view, comment on it, or merge it online at:

 #129

-- Commit Summary --

  • Display per-image titles/captions for galleries.
  • Keep the original link on the thumbnail.
  • Attempt to preserve the original urls when following links.

-- File Changes --

M lib/reddit_enhancement_suite.user.js (149)

-- Patch Links --

 https://github.com/honestbleeps/Reddit-Enhancement-Suite/pull/129.patch
 https://github.com/honestbleeps/Reddit-Enhancement-Suite/pull/129.diff


Reply to this email directly or view it on GitHub:
#129

@gamefreak

This comment has been minimized.

Show comment
Hide comment
@gamefreak

gamefreak May 2, 2012

Collaborator

Being able to click on the thumbnail at all is barely on my mind. I don't think I have ever even clicked on the thumbnail and I didn't know that it was a link until I saw people complaining about it not purpling their links.

The big issue here a collision of mindsets. I tend to hang around a community that is highly art/artist oriented and considers attribution to be far more important than most subs do. Since I'm used to attribution being serious business I have been treating it as such, hence why I find linking back to the original more important than coloring the browser history.

I expect that if deviantart didn't link back to the art page then you would see plenty of complaints about that too.

It's well known that you can't please everybody, but we need to try pick the way that pleases the most people. Since one of the nice things about computer programs is that they are easily reconfigurable, I say take the easy way out and pick both.

Just pick the default.

Collaborator

gamefreak commented May 2, 2012

Being able to click on the thumbnail at all is barely on my mind. I don't think I have ever even clicked on the thumbnail and I didn't know that it was a link until I saw people complaining about it not purpling their links.

The big issue here a collision of mindsets. I tend to hang around a community that is highly art/artist oriented and considers attribution to be far more important than most subs do. Since I'm used to attribution being serious business I have been treating it as such, hence why I find linking back to the original more important than coloring the browser history.

I expect that if deviantart didn't link back to the art page then you would see plenty of complaints about that too.

It's well known that you can't please everybody, but we need to try pick the way that pleases the most people. Since one of the nice things about computer programs is that they are easily reconfigurable, I say take the easy way out and pick both.

Just pick the default.

@honestbleeps

This comment has been minimized.

Show comment
Hide comment
@honestbleeps

honestbleeps May 2, 2012

Owner

I do agree with you about the artist credit philosophy - that's why I
suggested having "credits" be non-optional, and link to the original
page... so clicking the "main" link (or thumbnail) goes to the image..
but clicking the credits link goes to the page...

I'll also look at your idea of making it an option... Like you - I
NEVER would've thought people would click the thumbnail.. but finally
it came out that most of the people complaining about purple links
were, in fact, clicking thumbnails and/or the l+c link - both of which
I wasn't rewriting...

On Wed, May 2, 2012 at 5:35 PM, Scott McClaugherty
reply@reply.github.com
wrote:

Being able to click on the thumbnail at all is barely on my mind. I don't think I have ever even clicked on the thumbnail and I didn't know that it was a link until I saw people complaining about it not purpling their links.

The big issue here a collision of mindsets. I tend to hang around a community that is highly art/artist oriented and considers attribution to be far more important than most subs do. Since I'm used to attribution being serious business I have been treating it as such, hence why I find linking back to the original more important than coloring the browser history.

I expect that if deviantart didn't link back to the art page then you would see plenty of complaints about that too.

It's well known that you can't please everybody, but we need to try pick the way that pleases the most people. Since one of the nice things about computer programs is that they are easily reconfigurable, I say take the easy way out and pick both.

Just pick the default.


Reply to this email directly or view it on GitHub:
#129 (comment)

Owner

honestbleeps commented May 2, 2012

I do agree with you about the artist credit philosophy - that's why I
suggested having "credits" be non-optional, and link to the original
page... so clicking the "main" link (or thumbnail) goes to the image..
but clicking the credits link goes to the page...

I'll also look at your idea of making it an option... Like you - I
NEVER would've thought people would click the thumbnail.. but finally
it came out that most of the people complaining about purple links
were, in fact, clicking thumbnails and/or the l+c link - both of which
I wasn't rewriting...

On Wed, May 2, 2012 at 5:35 PM, Scott McClaugherty
reply@reply.github.com
wrote:

Being able to click on the thumbnail at all is barely on my mind. I don't think I have ever even clicked on the thumbnail and I didn't know that it was a link until I saw people complaining about it not purpling their links.

The big issue here a collision of mindsets. I tend to hang around a community that is highly art/artist oriented and considers attribution to be far more important than most subs do. Since I'm used to attribution being serious business I have been treating it as such, hence why I find linking back to the original more important than coloring the browser history.

I expect that if deviantart didn't link back to the art page then you would see plenty of complaints about that too.

It's well known that you can't please everybody, but we need to try pick the way that pleases the most people. Since one of the nice things about computer programs is that they are easily reconfigurable, I say take the easy way out and pick both.

Just pick the default.


Reply to this email directly or view it on GitHub:
#129 (comment)

@gamefreak

This comment has been minimized.

Show comment
Hide comment
@gamefreak

gamefreak May 2, 2012

Collaborator

Okay instead of tweaking things around more lets stop and think about what it does now and what it needs to do.

As of 0a56b0d:

  • L+C opens the original, doesn't mark history.
  • The main link is set to direct, switches to original for clicking/selection, does not mark.
  • The thumbnail keeps the original, doesn't mark.
  • Expando marks, provides link to original.

  • Create a function to add a URL to the queue without needing to involve the image..
  • Make L+C mark history on click.
  • Make thumbnail mark history on click.
  • Make the main link mark history on click.

Am I missing anything here?

Collaborator

gamefreak commented May 2, 2012

Okay instead of tweaking things around more lets stop and think about what it does now and what it needs to do.

As of 0a56b0d:

  • L+C opens the original, doesn't mark history.
  • The main link is set to direct, switches to original for clicking/selection, does not mark.
  • The thumbnail keeps the original, doesn't mark.
  • Expando marks, provides link to original.

  • Create a function to add a URL to the queue without needing to involve the image..
  • Make L+C mark history on click.
  • Make thumbnail mark history on click.
  • Make the main link mark history on click.

Am I missing anything here?

@honestbleeps

This comment has been minimized.

Show comment
Hide comment
@honestbleeps

honestbleeps May 2, 2012

Owner

I think that about covers it... I could be missing a technical
solution you have in mind, but if you do not rewrite the link, then
marking things in history on click is a shady proposition for anything
that's not super fast - as the page will be immediately unloaded
(unless you open in new tab)....

in related news: I did find a way to access XPCOM objects via Addon
SDK and add urls to history... so the two (by far) biggest browsers
now have efficient / non-hacky URL history tracking

On Wed, May 2, 2012 at 6:33 PM, Scott McClaugherty
reply@reply.github.com
wrote:

Okay instead of tweaking things around more lets stop and think about what it does now and what it needs to do.

As of 0a56b0d:

  • L+C opens the original, doesn't mark history.
  • The main link is set to direct, switches to original for clicking/selection, does not mark.
  • The thumbnail keeps the original, doesn't mark.
  • Expando marks, provides link to original.

  • Create a function to add a URL to the queue without needing to involve the image..
  • Make L+C mark history on click.
  • Make thumbnail mark history on click.
  • Make the main link mark history on click.

Am I missing anything here?


Reply to this email directly or view it on GitHub:
#129 (comment)

Owner

honestbleeps commented May 2, 2012

I think that about covers it... I could be missing a technical
solution you have in mind, but if you do not rewrite the link, then
marking things in history on click is a shady proposition for anything
that's not super fast - as the page will be immediately unloaded
(unless you open in new tab)....

in related news: I did find a way to access XPCOM objects via Addon
SDK and add urls to history... so the two (by far) biggest browsers
now have efficient / non-hacky URL history tracking

On Wed, May 2, 2012 at 6:33 PM, Scott McClaugherty
reply@reply.github.com
wrote:

Okay instead of tweaking things around more lets stop and think about what it does now and what it needs to do.

As of 0a56b0d:

  • L+C opens the original, doesn't mark history.
  • The main link is set to direct, switches to original for clicking/selection, does not mark.
  • The thumbnail keeps the original, doesn't mark.
  • Expando marks, provides link to original.

  • Create a function to add a URL to the queue without needing to involve the image..
  • Make L+C mark history on click.
  • Make thumbnail mark history on click.
  • Make the main link mark history on click.

Am I missing anything here?


Reply to this email directly or view it on GitHub:
#129 (comment)

@gamefreak

This comment has been minimized.

Show comment
Hide comment
@gamefreak

gamefreak May 3, 2012

Collaborator

How about instead of queueing up URLs in the browser window, send the URLs to the background page for all browsers.

The background page could then send a foreground tab URLs one by one.

Collaborator

gamefreak commented May 3, 2012

How about instead of queueing up URLs in the browser window, send the URLs to the background page for all browsers.

The background page could then send a foreground tab URLs one by one.

@honestbleeps

This comment has been minimized.

Show comment
Hide comment
@honestbleeps

honestbleeps May 3, 2012

Owner

that's a possibility.. for now I think I want to hold off on further
image viewer changes as a number of issues have cropped up... I've
fixed most of them, but I'm still struggling to figure out why, in
Firefox, animated gifs stutter / start over after a second.. really
weird ..

I want to get a new release out tonight because there are some kinda
critical bugs I want squashed so I can stop getting emails :-\

On Wed, May 2, 2012 at 7:53 PM, Scott McClaugherty
reply@reply.github.com
wrote:

How about instead of queueing up URLs in the browser window, send the URLs to the background page for all browsers.

The background page could then send a foreground tab URLs one by one.


Reply to this email directly or view it on GitHub:
#129 (comment)

Owner

honestbleeps commented May 3, 2012

that's a possibility.. for now I think I want to hold off on further
image viewer changes as a number of issues have cropped up... I've
fixed most of them, but I'm still struggling to figure out why, in
Firefox, animated gifs stutter / start over after a second.. really
weird ..

I want to get a new release out tonight because there are some kinda
critical bugs I want squashed so I can stop getting emails :-\

On Wed, May 2, 2012 at 7:53 PM, Scott McClaugherty
reply@reply.github.com
wrote:

How about instead of queueing up URLs in the browser window, send the URLs to the background page for all browsers.

The background page could then send a foreground tab URLs one by one.


Reply to this email directly or view it on GitHub:
#129 (comment)

@honestbleeps

This comment has been minimized.

Show comment
Hide comment
@honestbleeps

honestbleeps May 27, 2012

Owner

This issue seems to have been put to bed (outside of this pull request, so closing)... that was fun... I certainly underestimated some things in that little excursion.. eesh.

Owner

honestbleeps commented May 27, 2012

This issue seems to have been put to bed (outside of this pull request, so closing)... that was fun... I certainly underestimated some things in that little excursion.. eesh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment