-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Save the directory you're working in the session #3347
Conversation
👍 |
@owncloud/designers what do you think? I think it’s a really good idea to save state and the current folder. Lots of apps do this, and it’s way better than those who always just throw you into initial state. @luizhenriquepalacio I assume this only works for Files, storing the directory? Can you also make it remember which app is open? And just also cc'ing @icewind1991 @butonic @DeepDiver1975 for code check and opinions. :) |
Related to #494, also cc'ing @LukasReschke because there might be security concerns? |
@@ -24,6 +24,9 @@ | |||
// Check if we are a user | |||
OCP\User::checkLoggedIn(); | |||
|
|||
// Start session | |||
session_start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't manually start the session, lib/base takes care of that
This one is safe :) |
@luizhenriquepalacio are you still with us on this topic? THX |
@bantu in case @luizhenriquepalacio is not responding the next few days - please take over this task - THX |
@DeepDiver1975 @jancborchardt @luizhenriquepalacio It is not very clear to me what the intention of this patch is. What problems does this patch address? Why/how is post-patch behaviour better than pre-patch behaviour? Are there any downsides? See e.g. #3238 (comment) |
@bantu as I commented above:
So it’s good because it preserves the user’s flow. Navigating back to the main folder can still be done with one step through the breadcrumbs, whereas navigating down can be several steps and you have to find the folder again as well. |
@bantu any updates on this? |
@jancborchardt Negative. Busy with #4649. |
@bantu no worries. :) |
@PVince81 you wanna check this out? Or what do you think in general about the functionality? |
I don't see any problem we had before. It seems this PR has been opened 5 months ago so there might have been quite a few changes since then. On master/OC6 the current dir of the user is kept in the URL (and uses a special schema/handling for IE8 with hashchange/history API) so I don't see why we'd need to additionally store it in the session. I'm not sure we'd want the user to automatically land on the last used directory when opening the files app directly, either by clicking on the "Files" icon or opening the default URL (http://host/owncloud/). This might surprise users as it means the current URL doesn't point to a specific page/dir consistently. Keeping the last/recent dir could be achieved with a "list of recently used dirs" which might fit in the left panel suggested in #1936 so the user can quickly navigate back to the last used x folders. Also, this PR might mess up with the IE8 workarounds related to hashchange/history API as that one relies on "dir" not being set. (http://host/owncloud/#?dir=/test instead of http://host/owncloud/?dir=/test) |
Good points @PVince81. Closing hence. |
Save the directory you're working in the session