-
Notifications
You must be signed in to change notification settings - Fork 28
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
Grouping servers as tree view - completed #252
Conversation
…edit into wacki4/refactoring.
Hi @wacki4, thanks for your help. I'll look into it in the next few days. It would be good if you could fix the merge conflicts. |
I think is done. I made it by page, so could be some code mistake, but i think that everything is allright. |
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.
Hi @wacki4,
thank for your great work. I add some comments. Feel free to ask me if you don't understand something. The following points are global things that i could not comment in the source code.
Enhancement:
- Check source code formatting (.jsbeautifyrc)
- Treeview: sort folders before servers
- Folder structure: check circular relations (actually folder A can be a parent of folder B, but at the same time folder B can also be parent of folder A)
Error:
- Disable drag/drop handler for folders
- Changes on global storage object are used in tree view even if i doesn't save the configuration.
- Folder options in select will not be loaded if i open the configuration view. After i click the edit button and go back, the options will be added.
- After saving the configuration the folder and servers will not be added.
- When I click the edit folder button in the configuration and go back, the first server in the list is selected. This is confusing :)
lib/views/folder-view.js
Outdated
|
||
// Events | ||
server.getConnector().on('log', (msg) => { | ||
self.ftpLogView.addLine(msg); |
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.
ftpLogView doesn't exists in this class. Better create functions onDidServerAdd, onDidFolderAdd functions for this class. In treeview where you call the addServer function, you can also set this function as callback.
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.
I don't really know what to do :) So i removed ftplogger, and if You think that something need to change - fell free for change.
lib/helper/storage.js
Outdated
return self.servers; | ||
} | ||
|
||
getTree() { |
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.
Where you use this function? You never need the whole tree structure. You always only need the parent or children folder(s).
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.
I use it in tree view - i need to build somewhere a tree, so i thought that te best place to make it would be Storage helper, than tree view. Maybe we would need it in future somewhere else.
So i put some corrections, resolved checks i've set to resolved, not - made a comment. I think that i done everything. Additional to Your message: Enhancement:
Error:
|
I have idea for
Maybe if clicked cancle/close window reload from config all data? And on save - always save it? It would be simplest way to do it. |
Your idea could work like this. I will check and test your changes promptly. |
Most of the adjustments look good. Some things can still be optimized but I could do that later. During testing I noticed some mistakes: folder configuration
treeview:
ftp-remote-editable.js
|
About that - maybe simpler would be saving only folder part when folder editing? Because now, in Your way - it looks for me like if i save Folder, and after that cancle server config - it won't be save as i would predict. Maybe make a copy of servers for configuration view, and also copy of folders for folder configuration view? But there it could became a bug when we delete folder on folders view... This case look really hard as i see it. Maybe, we shouldn't combine this in current way, but make 'edit folders' on context menu on remote pane? |
And if we implement my suggestion and remove in the folder configuration the save butten. Then you could only manage the folders there but saving the whole configuration, including the server settings, would only happen in the server configuration. This would make everything logical again. |
So we only have 'Close button' on Folder Configuriation view, and if we made some error, we close this window and close ConfigurationView, and reopen it, and everything is allright after that? It looks good and logical. Only hard for now would be copy of Storage object, but it probably only look so hard ;) |
So maybe if we don't have save button over Folder Configuration View, reloading after closing and not saving ConfigurationView would be ok? It will make everything easier. |
Yes it would be ok. The result should be the same. |
So i will try to complete it this week. Some of problems i've resolved localy. |
…ng changes in config
I am to stubborn to get to sleep with such a problems in my head ;) I think that now everything. I will only remember You problem with button next to folder chooser, and strange box inside folder name input ;) I don't know how where is there a problem. |
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.
Circular relations between the folders still exist. I Think best place would be in folder-configuration-view.js in fillInputFields function. The rest of the changes look good.
Circular relations between the folders still exist Can You tell me how to reproduce circular relations? I've tried yesterday many combination but nothing allowed mi such a change. |
E.g. If you click on new 2 times, 2 folders will be created. Now I switch to folder 1 and assign folder 2 as parents. Now I switch to folder 2 and assign folder 1 as parents, which should not work. When saving it comes to an error because there is a circle relation. |
Ok i will try at home to reproduce and correct it. |
I didn't close review requesting which i didn't change/didn't found on code. Please check on Your side. I think that now we are really close. |
Thanks for your help. Everything looks good so far. I will try to solve the connection problems from the issues first. After that I will make the last adjustments. You helped me a lot. Thank you very much for that. |
Always ready to help, cannot wait for this release ;) Next things coming from me :) |
When do You plan release ? :) |
Unfortunately, it took a little longer to fix the connection errors. But now that they seem to have been fixed, I can take care of the implementation of new features. So I will try to start this week. I don't have an exact date for the release yet. It will be released when it is finished) ;) |
I know, i also make everything in my free time ;) I have now only problem cause on my home computer i have new config, so everytime i have to run atom from console :D And also waiting for all changes on master, to make new ones, cause now beacause of changes there is realy big mess between branches ;) |
Hi @wacki4 , I have now made the adjustments in the branch "feature/grouping-servers". Can you test everything again from your side? |
Only problem that i found - when i've added one folder, get to servers, set on some of them this folder, got back to folders and added child folder for previous one - it wasn't seen on folders in server config. after save and getting back to server config - it was seen correctly. Also, when deleting folder with servers, and folder has parent - maybe better would be to move servers to parent folder? It should not be hard to done. |
Completed saving and viewing for tree folders view.
TODO:
Regarding to #72