Expand sidebar's rename functionality #2234
Conversation
|
@sbauer322 Just wanna say that you should take into account the discoverability. Windows users don't make the connection between rename and move as linux users do—due to the First thing first have something that works.
Id recommend the human interface guidelines of Elementary OS. Of course you have to take into account the peculiar user base of LT. There's a big emphasis on the keyboard; in that regard:
Hope that helps. |
I think a drop down—using auto complete—of the existing folders at that depth would make more sense. The user doesn't want to create folders he wants to either rename the file or move it to an existing folder. So the minimum would be to disable the mv if there's an inexistant folder (and add a red dotted line under the culprit). The harder solution would be to restrict the selection of the folders to the auto-complete suggestions. |
|
While I do agree these would be nice things, I do not feel they would be within the scope of this issue or appropriate to a essentially a first iteration. That said, I intend to open additional issues recording those improvements after the PR is accepted so they can be addressed in the future. |
|
So you intend to recursively mkdir? IMHO this is too much; KISS and just log an error if a folder doesn't exist in the path. |
|
You do have a good point about it likely being error prone. I have removed the ability to add arbitrary folders as you have suggested. It should be pushed here sometime in the next few days. |
sbauer322
force-pushed
the
feature-add-rename-file
branch
from
7c4adda
to
0cb8599
Compare
|
@kenny-evitt regarding the issue mentioned in Gitter: This appears to be a manifestation of preexisting issue #2129 and not a direct result of this pull request. I have commented on that issue. When renaming a file or folder at the root folder level (not sure if that is the correct phrase, but I mean the highest point in a workspace) the file or folder disappears until the root folder is manually refreshed. Despite my best efforts, I cannot figure out why it is occurring only at this level - all other folder depths work properly... As far as I can tell, renaming files and folders all work properly except for this level of folders. Would you have any ideas? Here are some screenshots illustrating the problem: Initial state - README.md is displayed in the sidebar properly
After renaming README.md to READMEs.md the file vanishes from the sidebar:
After manually refreshing the workspace folder READMEs.md will appear again:
|
|
The new file UI is interesting but I wasn't expecting it to look different:
I confirmed that I can move a file by renaming it – so that's good! I'm going to stew on this for a while before I decide how I feel about it. Right now I'm conflicted. For those wanting to be able to their mouse and right-click, let alone drag-and-drop, this doesn't feel satisfying. And for those that wouldn't mind moving files by 'renaming' them, I'd expect they're already satisfied by just doing that thru a separate command line terminal program. @rundis @cldwalker Your thoughts are most welcome. |
|
Yes, I agree those that want to use a mouse would not be satisfied. As mentioned at the start (via Gitter), I was using Atom as inspiration for this - they have something similar to what was implemented as well as the drag and drop functionality. This PR also aligns with the first bullet from the original issue #2218 If the UI side is what is causing the conflict then it can be changed. I don't feel strongly about it... I would just need some idea of what it should look like instead. I'm happy to include drag and drop as well but figured it should be a separate pull request at some point - there were a lot of changes for a first time PR and I did not want to go overboard only to find out I was missing some critical thing. |
|
Well normally if you want to move a file, you would expect a dialog box where you can navigate to select the destination folder. Of course all the files are hidden—only the folders are selectable. related: webkitdirectory attribute |
|
Totally agree and that was my first attempt! However, all of LT's dialog boxes (e.g., save, open) come from electron and I did not discover one for moving files. |
|
The process would look like that:
|
|
@sbauer322 I forgot about the Atom inspiration. That does lend support for it not being completely foreign to everyone. How do you feel about hiding this behind a feature toggle, i.e. a setting that needs to be explicit set to enable this new UI feature? Here's the Atom screenshot Scott shared in Gitter earlier (for anyone interested): Can you share a full-window screenshot with the rename/move file UI from Atom too? I'm just curious what the visual difference is with the UI you've implemented. |
Obviously shouldn't contain the file name as shown in the screenshot. |
|
Here you go @kenny-evitt:
Atom obviously has a more polished rename UI, but getting this UI hammered out is part of this PR too! The rename UI was done rather simply since it was not clear how it should appear... ought be fairly straightforward to make changes. |
|
With regards to having an explicit option to set for this to be enabled, I feel such a thing would be against the spirit of the original issue - they simply want some way to easily move files around within the IDE. Once drag and drop for files and folders is implemented then I would be more comfortable relegating this to an explicit option. |
|
@sbauer322 I created a new post in the Google Groups group asking for help testing and soliciting feedback about this. I kinda like how your version of the UI is docked to the edge of the workspace sidebar/pane versus the Atom version being centered in the editor tab(s). |
|
Any chance we can merge this, @kenny-evitt? The Google Group post hasn't garnered any suggestions, or negative feedback, so I guess we are in the clear? We can always add updates later on. |
|
@sbauer322 GitHub claims these commits conflict with the master branch. Please rebase master on your PR topic branch and fix any conflicts. @rundis Mind reviewing this when you get a chance? I'm mostly okay with this but I'd prefer it 'hidden' behind a feature toggle so as to not surprise existing users. I'll defer to your judgement tho. |
sbauer322
force-pushed
the
feature-add-rename-file
branch
from
12a4a4c
to
e53a380
Compare
|
@rundis, any chance I could get your feedback for this? I'm happy to make adjustments as needed... though I still feel this should not be hidden behind a feature toggle, I will do so if necessary. One (possibly absurd) thought might be to extract all rename functionality as a whole into its own plugin... essentially start the process of decomposing Light Table itself into a series of plugins (there would be a core still, obviously). |
Is there relevant discussion on how this was decided? Not seeing in issues which is where any issue related discussion and decision should be happening. I'm not leaning towards this approach
To that point, feature enhancements like this should be avoided in core unless we have a large user demand for it (which we don't here). I'm happy to consider updating LightTable core so that it could support a topbar plugin. Thoughts on what would need in core to support such a plugin? |
|
Thanks for the 0.8.2 milestone! You should check that thread on the mailing list.
I didn't see you participate in that thread: https://groups.google.com/forum/#!topic/light-table-discussion/EP8bL-NhyME |
|
Have to say, you are sending mixed signals by adding this to the milestone list, but preferring it moved into a plugin, which would entail closing this PR! 😉 I feel there are areas to improve with the PR, but I would first need to know what our plans are.
Regarding demand for it, not having a way to move files around in Light Table itself is somewhat embarrassing. I can't think of any modern editor, let alone one listed in the top editors on Github, that don't have some version of this functionality... not to mention we like to describe ourselves as a "next generation code editor". Drag and drop is an option, and should be added too, but we need to support keyboard based movement as well. Whether this movement functionality is added as a plugin or to the core, I am not as concerned about, but they do need to be added.
I want to say the discussion was either in Gitter, or simply decided on my own... the pull request itself was my first significant work on Light Table (prior to becoming a Light Table maintainer) and it was a way to learn about the internals. Since this enhanced rename contains the functionality of the original rename, I don't see much benefit in keeping them both.
As for adding a topbar, if you would rather see this as plugin, it shouldn't be that much different than our existing bars... something of a mix between bottombar, and popup. I don't know enough about plugins (yet) to say what would need to be exposed by Light Table to do this in a plugin, however. If we went down this path, we will need to close this PR, open a new one with just the topbar, and then create a rename plugin that can override/suppress Light Table's existing rename functionality. In any case, I feel a topbar would be beneficial to other areas of the project. |
Light Table has abilities to rename a specific file and folder so I imagine it's that specific desire to move files across folders that you're looking for. General calls to feature parity and embarrassment aren't convincing Looking at this PR more, there doesn't seem to be any major hooks for LT to provide for this as a 3rd party plugin. If I am missing something on that front, let's discuss. Overall I'm glad you're passionate about this functionality and that it could help some users. I'd recommend first publishing this as a personal plugin and pointing users to it in #2234. Over time we can discuss why this functionality merits being in core more than other 3rd party plugins. Feel free to close or I can soon |
That's an unusual and peculiar stance. If he was asking for a diff/compare tool to be added to the core I would understand your position though. I am puzzled by your plugin request. We probably have very different ideas of what is minimal. |
|
Sorry, didn't mean to come across as particularly passionate on this topic... In the earlier comment, perhaps frustrating would have been more appropriate than embarrassing. In any case, yes, being able to move files and folders around is what I was I looking to get out of this PR. As for merit to include this in core, we already have similar functionality in the core, which is our current rename. Splitting the two - with one in core and the other a plugin - seems odd. It should be one way or the other. But all this goes back to if we wanted to pursue breaking up Light Table functionality, such as rename/move file and searching, into their own plugins. Additionally, #2218 was labelled as an enhancement as opposed to being closed or added to our plugin wishlist. Beyond what was said above, I am not going to try attempting to convince anyone of the merit of this to be in core - it simply is not that big of an issue to squabble over. I'm fine with closing this and relegating it to a plugin someday. |
@sbauer322 I set up these labels. An enhancement label isn't a reason or a substitution for a discussion. An enhancement is an "Interesting feature suggestion which isn't on the roadmap but welcome". If you're using labels in a different way, please define them somewhere |
|
I'm not sure what more I can say, but there was quite a lot of discussion, and it was okayed to implement in Light Table core and to expand the current rename functionality. Some relevant links to the Gitter discussion, which is where we are directed to post questions and other transient information:
Regarding the label, I am aware of that definition and it is fine... my point was that it is misleading to label an issue as an enhancement or feature while expecting it to actually be a plugin. It only helps to cause confusion and we should be more clear about what issues we expect a plugin rather than a pull request. |
Our policy on feature discussion is pretty explicit. Apologies if you were lead to think that didn't apply for this case. For future reference, gitter is fine for support and realtime discussion but ultimately LightTable work needs to be coordinated on github
The whole point of the label descriptions is to minimize this confusion. Naming alone doesn't provide any of the context on how the core team processes these labels. I've brought back the descriptions on the wiki. Please update the ones you've changed. If you have better names for the |
|
The label names have been restored. |
The Pull Request is for issue #2218. Rather than add a new, somewhat redundant, move function to the context menu in the sidebar, it was decided to expand the rename functionality.
It should be possible to now edit the file/folder name as well as the location of the file/folder within the filesystem. Below is an example of the UI at the start of the PR:
This is currently a Work in Progress - looking for feedback! 😄