Project Rework #1455
Project Rework #1455
Conversation
|
OK; I think that's most of it. I'm going to use this PR as my daily driver over the next few days, and will fix bugs as they come up. |
|
Some residue of old system here: Lines 311 to 315 in 687db40 I guess it should be: elseif not core.projects[1] then
local cwd = system.absolute_path(".")
return cwd .. PATHSEP .. common.normalize_path(filename)
else
return core.projects[1].path .. PATHSEP .. filename Edit: |
|
Good catch, removed. |
|
Various fixes to restore broken treeview functionality with new projects system: diff --git a/data/plugins/treeview.lua b/data/plugins/treeview.lua
index 5f9ca0b0..67353fa8 100644
--- a/data/plugins/treeview.lua
+++ b/data/plugins/treeview.lua
@@ -106,7 +106,7 @@ function TreeView:get_cached(project, path)
end
if t.expanded and t.type == "dir" and not t.files then
t.files = {}
- for i, file in ipairs(system.list_dir(path)) do
+ for i, file in ipairs(system.list_dir(path)) do
local l = path .. PATHSEP .. file
local f
if self.show_ignored then
@@ -119,7 +119,7 @@ function TreeView:get_cached(project, path)
f.abs_filename = l
table.insert(t.files, f)
end
- self.cache[l] = nil
+ self.cache[l] = nil
end
table.sort(t.files, function(a, b) return system.path_compare(a.name, a.type, b.name, b.type) end)
end
@@ -555,9 +555,9 @@ end
core.add_thread(function()
while true do
for k,v in pairs(view.watches) do
- v:check(function(directory)
- view.cache[directory] = nil
- end)
+ v:check(function(directory)
+ view.cache[directory] = nil
+ end)
core.redraw = true
end
coroutine.yield(0.01)
@@ -604,11 +604,9 @@ function core.on_quit_project()
on_quit_project()
end
-local function is_project_folder(path)
- for _,dir in pairs(core.projects) do
- if dir.name == path then
- return true
- end
+local function is_project_folder(item)
+ if item.abs_filename == item.project.path then
+ return true
end
return false
end
@@ -629,7 +627,7 @@ menu:register(function() return core.active_view:is(TreeView) and treeitem() end
menu:register(
function()
local item = treeitem()
- return core.active_view:is(TreeView) and item and not is_project_folder(item.abs_filename)
+ return core.active_view:is(TreeView) and item and not is_project_folder(item)
end,
{
{ text = "Rename", command = "treeview:rename" },
@@ -653,7 +651,7 @@ menu:register(
local item = treeitem()
return core.active_view:is(TreeView) and item
and not is_primary_project_folder(item.abs_filename)
- and is_project_folder(item.abs_filename)
+ and is_project_folder(item)
end,
{
{ text = "Remove directory", command = "treeview:remove-project-directory" },
@@ -677,7 +675,7 @@ command.add(nil, {
view.show_ignored = not view.show_ignored
view.cache = {}
end,
-
+
["treeview:toggle-focus"] = function()
if not core.active_view:is(TreeView) then
if core.active_view:is(CommandView) then
@@ -807,7 +805,7 @@ command.add(
local relfilename = item.filename
if item.project ~= core.root_project() then
-- add secondary project dirs names to the file path to show
- relfilename = common.basename(item.dir_name) .. PATHSEP .. relfilename
+ relfilename = common.basename(item.abs_filename) .. PATHSEP .. relfilename
end
local file_info = system.get_file_info(filename)
local file_type = file_info.type == "dir" and "Directory" or "File"
@@ -875,15 +873,17 @@ command.add(
end,
["treeview:new-file"] = function(item)
- local text
- if not is_project_folder(item.abs_filename) then
+ local text, path
+ if not is_project_folder(item) then
text = item.filename .. PATHSEP
+ path = common.dirname(item.abs_filename)
+ else
+ path = item.project.path
end
core.command_view:enter("Filename", {
text = text,
submit = function(filename)
- local doc_filename = item.dir_name .. PATHSEP .. filename
- core.log(doc_filename)
+ local doc_filename = path .. PATHSEP .. filename
local file = io.open(doc_filename, "a+")
file:write("")
file:close()
@@ -891,25 +891,28 @@ command.add(
core.log("Created %s", doc_filename)
end,
suggest = function(text)
- return common.path_suggest(text, item.dir_name)
+ return common.path_suggest(text, item.abs_filename)
end
})
end,
["treeview:new-folder"] = function(item)
- local text
- if not is_project_folder(item.abs_filename) then
+ local text, path
+ if not is_project_folder(item) then
text = item.filename .. PATHSEP
+ path = common.dirname(item.abs_filename)
+ else
+ path = item.project.path
end
core.command_view:enter("Folder Name", {
text = text,
submit = function(filename)
- local dir_path = item.dir_name .. PATHSEP .. filename
+ local dir_path = path .. PATHSEP .. filename
common.mkdirp(dir_path)
core.log("Created %s", dir_path)
end,
suggest = function(text)
- return common.path_suggest(text, item.dir_name)
+ return common.path_suggest(text, item.abs_filename)
end
})
end,
@@ -946,10 +949,10 @@ command.add(function()
local item = treeitem()
return item
and not is_primary_project_folder(item.abs_filename)
- and is_project_folder(item.abs_filename), item
+ and is_project_folder(item), item
end, {
["treeview:remove-project-directory"] = function(item)
- core.remove_project(item.dir_name)
+ core.remove_project(item.project)
end,
})
|
|
Sure, looks good. |
|
More recommended fixes: Restores proper behavior of calls to function core.set_project(project)
while #core.projects > 0 do core.remove_project(core.projects[#core.projects], true) end
- return core.add_project(project)
+ local project = core.add_project(project)
+ system.chdir(project.path)
+ return project
endFixes many plugins overrides triggered after core.exit is called and which depend on core.root_project() or core.projects[1] returning a project instead of nil. function core.exit(quit_fn, force)
if force then
core.delete_temp_files()
- while #core.projects > 0 do core.remove_project(core.projects[#core.projects], true) end
+ while #core.projects > 1 do core.remove_project(core.projects[#core.projects]) end
save_session()
quit_fn()
else |
|
So with regards to If we don't Re: the secondary change; I'll review this, but you're probably right. We should probably always have a project. |
I see
Not only plugins but also some minor core parts need adjustment:
local function add_config_files_hooks()
-- auto-realod style when user's module is saved by overriding Doc:Save()
local doc_save = Doc.save
local user_filename = system.absolute_path(USERDIR .. PATHSEP .. "init.lua")
function Doc:save(filename, abs_filename)
- local module_filename = system.absolute_path(".lite_project.lua")
+ local module_filename = core.project_absolute_path(".lite_project.lua")
doc_save(self, filename, abs_filename)
if self.abs_filename == user_filename or self.abs_filename == module_filename then
reload_customizations()
core.rescan_project_directories()
core.configure_borderless_window()
end
end
end
function common.dir_path_suggest(text, root)
local path, name = text:match("^(.-)([^/\\]*)$")
- local files = system.list_dir(path == "" and (root or ".") or path) or {}
+ path = path == "" and (root or ".") or path
+ path = path:gsub("[/\\]$", "") .. PATHSEP
+ local files = system.list_dir(path) or {}
local res = {}
for _, file in ipairs(files) do
file = path .. file
diff --git a/data/plugins/treeview.lua b/data/plugins/treeview.lua
index 5fdd95bf..31dd8655 100644
--- a/data/plugins/treeview.lua
+++ b/data/plugins/treeview.lua
@@ -434,7 +434,7 @@ function TreeView:draw()
local _y, _h = self.position.y, self.size.y
local doc = core.active_view.doc
- local active_filename = doc and system.absolute_path(doc.filename or "")
+ local active_filename = doc and core.project_absolute_path(doc.abs_filename or "")
for item, x,y,w,h in self:each_item() do
if y + h >= _y and y < _y + _h thenOn the plugins side so far I have been adjusting them on pragtical repos branch to work with the changes from this PR and have gotten to adjust:
And additionally on the file picker |
|
To be clear; this PR is fully open to comments and stuff; do you have any thoughts on the change to |
From my point of view I don't see any immediate or evident benefits from not doing a In my opinion I would keep |
|
I don't disagree; it's an entirely reasonable POV. My only concern would be that this would necessitate two version bumps, rather than one, which does make the plugin transition more bumpy. Let's see what @Guldoman has to say. |
data/core/common.lua
Outdated
| @@ -172,6 +172,7 @@ function common.path_suggest(text, root) | |||
| local s, e = file:find(root, nil, true) | |||
| if s == 1 then | |||
| file = file:sub(e + 1) | |||
| table.insert(res, file) | |||
There was a problem hiding this comment.
| table.insert(res, file) |
Causes duplicate entries
|
Another issue I came across with the treeview plugin while building a package that removed and regenerated some directories causing a crash: if t.expanded and t.type == "dir" and not t.files then
t.files = {}
- for i, file in ipairs(system.list_dir(path)) do
+ for i, file in ipairs(system.list_dir(path) or {}) doAlso noticed that some plugins that launch a process expect the current working directory to be that of the main project, adjusted them to use the |
data/plugins/treeview.lua
Outdated
| v:check(function(directory) | ||
| view.cache[directory] = nil | ||
| end) | ||
| core.redraw = true |
There was a problem hiding this comment.
| core.redraw = true |
I was noticing higher cpu usage and one of the reasons was this
|
AutoReload plugin is causing high cpu usage, to reproduce using this branch:
|
* Initial commit to clean up projects; spun off find-file to its own plugin, removed project limit, removed the concept of a project maintaining an ordered list of files, and allowed treeview to see things like hidden files and files not actually in the project. * Normalizing things, fixed typo. * Abstracted root project, and made things more in line with current master behaviour. * Removed unused legacy code, as well as ensured that we use absolute paths. * Fixed issue with backslahes on linux, will look at windows at some point. * Removed stray print. * Removed orphaned function. * Removed extraneous command. * Fixed the ability to close project folders. * Adaptations for project rework. * Removed superceded function. * Applied jgm's suggestions.
|
Sharing some fixes I did after merging to pragtical if interested: Also all pragtical repositories like: plugins, console, etc... were adapted to work with new project changes. |
|
OK, sounds good; will probably pull in some of those fixes; we'll want to make sure this fixes #1183. |
|
Another change that may be of interest: pragtical/pragtical@a5df5be Edit: also bug fix on |
There was a problem hiding this comment.
I looked over the code and it looks good overall.
Tested it a fair bit too, but lite-xl is not my daily driver (yet, working on fixing that) so its possible I missed some edge cases, I encourage other people try this PR out too, even if they don't want to or can review the code.
The code itself looks pretty good too, here are some keynotes on what was changed:
- mod version was bumped to 4, many old interfaces should be directly compatible with the new system but the change is still quite huge and plugin developers will need to check and possibly update their code
- Projects are no longer just an string attribute of core but an actual object with which you can do a lot of project logic directly
- there can now be multiple open projects at once
- existing logic that depends on only 1 project existing uses the first project available (the root project)
- some code that expected only one root directory to exist can now accept others with a fallback to the original behavior where needed
- storage access was abstracted into an object. As far as I can tell there is only 1 default storage driver right now with no way to change the driver on a per-project or global level, but I think this PR is already big enough and this should be tackled in a future PR. Overall a storage interface is great for unique implementations or secure access interfaces e.g. Android/iOS scoped Storage, flatpak Portals, external foreign systems (e.g. MicroPython vfs), remote system access (ssh, ftp)
Some info for testers:
core:open-project-folderopens a new instance, not add a new project to the existing instancecore:add-directoryadds a new project to the existing instance
This is a bit unintuitive if you don't know about it
There was a problem hiding this comment.
Initial incomplete review.
Ideally a bunch of things should be moved to their own PR (TreeView features, Storage).
The main thing that worries me a bit is that we don't have a single source of truth for the filesystem tree, so there is some duplication with TreeView and findfile.
Also considering that findfile needs to visit the tree every time it's called. Maybe it should cache the entries and immediately propose them, while updating them in the bg.
There was a problem hiding this comment.
Should we just do away with the filename/abs_filename thing?
Considering that . is not useful anymore.
There was a problem hiding this comment.
Yes; I think that'd be fine. This, is, however a huge change. Can we keep this for now, and have this in a separate PR?
There was a problem hiding this comment.
Yeah, that definitely belongs to a separate PR.
Should I open an issue so that we don't forget?
data/core/init.lua
Outdated
| function core.normalize_to_project_dir(path) return core.root_project():normalize_path(path) end | ||
| function core.project_absolute_path(path) return core.root_project() and core.root_project():absolute_path(path) or system.absolute_path(path) end |
There was a problem hiding this comment.
I wonder if we should just avoid these, so that people think if they actually want the root project or a more appropriate one.
Should we add a function that tries to return the correct project for a file path?
There was a problem hiding this comment.
These are here as legacy functions so we don't have to go in any fix a ton of stuff. I can add a deprecation warning, if you like.
And sure; would probably be reasonable to get a project with a path. Done.
…et has_restarted.
adamharrison
force-pushed
the
PR/project-rework
branch
from
e068195 to
8e0ae38
Compare
…ion warnings for legacy interface.
This is to be expected. They do different things, fundamentally. The treeview should only ever be meant to be looking at the very top level of the tree. It doesn't have any business looking deeper. It also looks at ignored entries, hidden entries, and other things that findfile may want to ignore. Conversely, findfile is expected to be more exhaustive, yet obey more of the rules when looking at treeview. It also needs less information, though. It makes sense to split these; part of the problems we've had in the past has been attempting to unify this kind of disparate information into a single structure, and keeping it updated, which doesn't make sense for most usecases.
That I'm OK with. It's honestly speedy enough for me though in all the cases I've checked; so I'd say we should propose that as an enhancement in another PR. |
| function core.project_for_path(path) | ||
| for i, project in ipairs(core.projects) do | ||
| if project.path:find(path, 1, true) then return project end | ||
| end | ||
| return filename | ||
| return nil |
There was a problem hiding this comment.
Would it make sense to return the longest match?
There was a problem hiding this comment.
Don't think it matters; unless you have nested projects, which I presume we do not support.
There was a problem hiding this comment.
The changes in TreeView with the hidden/ignored stuff.
| @@ -210,7 +204,7 @@ function ResultsView:draw() | |||
| renderer.draw_rect(x, y, w, h, style.line_highlight) | |||
| end | |||
| x = x + style.padding.x | |||
| local text = string.format("%s at line %d (col %d): ", item.file, item.line, item.col) | |||
| local text = string.format("%s at line %d (col %d): ", core.root_project():normalize_path(item.file), item.line, item.col) | |||
There was a problem hiding this comment.
I'm not sure I follow.
I'm referring to the fact that you can trigger the search from any directory, so if the search started from a directory not in the root project, the resulting filename would look wrong.
There was a problem hiding this comment.
Yeah, that definitely belongs to a separate PR.
Should I open an issue so that we don't forget?
|
As part of this, any thoughts about also just adding the following: diff --git a/data/core/process.lua b/data/core/process.lua
index e6445356..d7254a41 100644
--- a/data/core/process.lua
+++ b/data/core/process.lua
@@ -164,6 +164,7 @@ local old_start = process.start
function process.start(command, options)
assert(type(command) == "table" or type(command) == "string", "invalid argument #1 to process.start(), expected string or table, got "..type(command))
assert(type(options) == "table" or type(options) == "nil", "invalid argument #2 to process.start(), expected table or nil, got "..type(options))
+ if options.cwd == nil then options.cwd = core.root_project().path end
if PLATFORM == "Windows" then
if type(command) == "table" then
-- escape the arguments into a command line stringOf course, passing |
|
Potential initialization issue with core.process requiring core before everything is loaded. |
Yeah; fixable by just lazy loading in core. But if you're of the opinion we don't need it, I'm cool with just leaving it off. |
Co-authored-by: Guldoman <giulio.lettieri@gmail.com>
Introduces the concept of projects as a class to lite-xl, rather than the ad-hoc
project_dirswe have now.This also removes the concept of keeping the ordered file list for projects under the file limit, which has major advantages, and a small amount of disadvantages.
Advantages
core:find-fileis never removed entirely, even on large directory trees; it simply loads things as needed. This functions well even up to very large directory trees. It allows allows us to place this as a plugin, rather than as a core feature.project_subdir_is_shownfromcore, which shouldn't really be there (it was basically written for TreeView specifically).Disadvantages