Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If there is no browser history, exit the slideshow by calling stop. #468

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

setnes
Copy link
Contributor

@setnes setnes commented Dec 5, 2015

This is a fix for #460.

This is the same code that was in #463.

I am renaming branches. Sorry for any confusion. :)

@oparoz oparoz added this to the 9.0-current milestone Dec 5, 2015
@oparoz
Copy link
Contributor

oparoz commented Dec 6, 2015

This doesn't seem to work for me, but maybe it's a corner case...

  1. Share a folder containing 1 JPEG
  2. Go to the link
  3. Click on the image
  4. Paste that link in another browser
  5. Click close

Both with and without the patch, I end up with an empty tab

@setnes
Copy link
Contributor Author

setnes commented Dec 6, 2015

I think I need to explain this. I'm trying to fix something very similar, but not exactly what you are testing.

  1. Your browser history is not empty in your test. It's subtle, but your first history entry is actually your browser start page.
  2. Your test case actually works the way I think it should work. You went back a page in your history when you clicked the X. To me this seems natural.
  3. What I am fixing is when there is NO history... like when you click on a link in an e-mail and a new browser window or tab is opened. When there is NO history, clicking the X used to do nothing. You were essentially stuck in the page. Now it closes the slideshow image and you are back in a gallery.

I don't think you can actually read history entries. That's by design. They don't want you to know what other pages the user has been to.

@oparoz
Copy link
Contributor

oparoz commented Dec 6, 2015

  1. ...To me this seems natural.

To me it doesn't :). As a user, if I'm given a link and there is a X. I should be able to click on it and get the photowall or files list view.
I reckon it's going to be difficult to do without rewriting all the logic, because clicking on the back button should go to the empty page and not just exit the slideshow.

@setnes
Copy link
Contributor Author

setnes commented Dec 6, 2015

I don't think we need to rewrite everything. But I think we have two different issues. This pull very much fixes one of the two issues and leaves the other one unchanged.

Maybe I'll think about this further and see if there is a way to make this work the way you want it without a big rewrite. One thought I had was to set a var once we have visited any album page and test that along with the history. If it is unset, we may have entered from a direct photo link.

@oparoz
Copy link
Contributor

oparoz commented Dec 6, 2015

The problem with the var is that the photowall is loaded in the background, so it will be set as soon as you open the slideshow.

@setnes
Copy link
Contributor Author

setnes commented Dec 6, 2015

True... but now that we are writing click handlers for navigation, we can set something to know the user entered the slideshow from the gallery or not before navigating. :)

slideshowOpenedFromAlbum = true;

(yeah... probably need a shorter name)

@oparoz
Copy link
Contributor

oparoz commented Dec 6, 2015

probably need a shorter name

Yeah, this is not Java :D

But OK, so if clicked, then use history, else just close the slideshow.

@setnes
Copy link
Contributor Author

setnes commented Dec 6, 2015

What about from the files page instead of a gallery album page? Will we be able to set something in that case?

I actually think there might be an easier fix for this. It's very important that navigating back calls stop(). It's not as important for _exit to call window.history.back though. It could just as easily call stop() instead.

@oparoz
Copy link
Contributor

oparoz commented Dec 6, 2015

You're welcomed to give it a go. All I remember is that it took me days to find a working solutions. There was always something which would break under a certain condition. Maybe a variable is the simple solution which always works :)

The specs are still here, so it should help with the debugging.

One thing to keep in mind is that on 8.2+ things have changed in the Files app, using # doesn't reset the app any more.

@setnes
Copy link
Contributor Author

setnes commented Dec 7, 2015

I worked on this a little, and now agree that a variable is going to be easier. :) This will require code changes that are being made in #471. Should I just add it to that change, or should this wait for it?

Some notes for when I can work on this again...

  • You cannot successfully start the slideshow from an image link that uses an "apps/files" path. Right now, we only need to worry about image links that use an "apps/gallery" path.
  • The Gallery object exists only when the "apps/gallery" path is used. When opening the slideshow from the files view, the Gallery object is not defined. (this will be helpful)
  • We can create a Boolean named Gallery.imageClicked with a default value of false.
  • In the new _openImage function from Click handlers for image and album thumbnails. #471, we can set Gallery.imageClicked to true.
  • In the _exit function, we should only call window.history.back() IF...
    • We can manipulate history.
    • We have more than 1 history entry.
    • Gallery is not defined OR ( Gallery is defined AND imageClicked is true )
  • Otherwise we call this.stop() to close the slideshow and get to a gallery view.

@oparoz
Copy link
Contributor

oparoz commented Dec 9, 2015

Let's finish #471 before this one then.

@oparoz oparoz mentioned this pull request Feb 11, 2016
8 tasks
@oparoz oparoz modified the milestones: 9.1-next, 9.0-current Feb 11, 2016
@oparoz
Copy link
Contributor

oparoz commented Feb 12, 2016

You can now create a PR against the next-9.1 branch if you want to try and fix this.

@setnes
Copy link
Contributor Author

setnes commented Feb 13, 2016

I do want to fix this and will give it a shot. It won't happen this weekend though. I'm still here though. :)

@oparoz
Copy link
Contributor

oparoz commented Feb 13, 2016

Cool :). I'll release a new version of Gallery+ for 8.2 in the coming weeks, but there will be another one in March.

@oparoz oparoz modified the milestones: 9.2-next, 9.1-current May 31, 2016
@felixboehm felixboehm modified the milestones: oC 9.2 / NC 11, development May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants