-
Notifications
You must be signed in to change notification settings - Fork 65
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
base: master
Are you sure you want to change the base?
Conversation
c885480
to
5ee22c3
Compare
This doesn't seem to work for me, but maybe it's a corner case...
Both with and without the patch, I end up with an empty tab |
I think I need to explain this. I'm trying to fix something very similar, but not exactly what you are testing.
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. |
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 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. |
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. |
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) |
Yeah, this is not Java :D But OK, so if clicked, then use history, else just close the slideshow. |
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. |
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. |
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...
|
Let's finish #471 before this one then. |
You can now create a PR against the next-9.1 branch if you want to try and fix this. |
I do want to fix this and will give it a shot. It won't happen this weekend though. I'm still here though. :) |
Cool :). I'll release a new version of Gallery+ for 8.2 in the coming weeks, but there will be another one in March. |
This is a fix for #460.
This is the same code that was in #463.
I am renaming branches. Sorry for any confusion. :)