Skip to content
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

Feature: add the state's last modified date/time in the fuzzy search dialog #82

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

ChrisGVE
Copy link

@ChrisGVE ChrisGVE commented Dec 26, 2024

  1. The modified code adds a new date/time column within the label field passed to the fuzzy dialog. After the glob is retrieved by wezterm, the last change of each file is retrieved. The maximum length is derived from the list of filenames such that a proper column is formatted with a padding of '.' so it is easy to read. The format functions have been modified to display the date/time in white, leaving the color focus on the name of the state.
  2. I've noticed that when changing the state_save_dir, the proper subfolders (tab, workspace, window) are expected to be created as well, which is non-intuitive, in my opinion. Thus, I have modified the code such that when the folder is changed, the plugin ensures that the necessary folders are present and, if needed, creates them.

I've attempted to write the changes for Windows and Mac/Linux, but I wasn't able to test thoroughly for Windows.

This PR closes issue #80

When `chahge_save_state_dir` is used, the code now ensures that the
necessary folders are created to prevent errors for non-existing
folders.
- Create the folders "workspace", "window", "tab" directly under the
  selected folder instead of under an intermerdiary "state" folder.
- Trim and normalize the provided folder name
fix(save_state_dir): create file structure for windows/linux/mac

- Both features should work for windows/linux/mac, but I couldn't
  test thoroughly on windows.
Copy link
Owner

@MLFlexer MLFlexer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for creating this PR! It is really awesome that you made the issue and the fix aswell, I really appreciate it!

I have added some comments about the date commands and some minor changes to the Lua comments.

I am also wondering if this version of the fuzzy finder is noticably slower, or do you not notice the difference?

Lastly, i think it would be greate to have an option to opt out of using the dates, this could be by just making this into it's own seperate fuzzy finder function, or extening the @param opts fuzzy_load_opts? with an option which skips the dating, if the option is set.

plugin/init.lua Outdated
Comment on lines 417 to 418
-- Convert the date format into "dd-mmm-yyyy HH:MM"

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-- Convert the date format into "dd-mmm-yyyy HH:MM"
-- Convert the date format into "dd-mm-yyyy HH:MM"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For your consideration: I prefer to show date either as yyyy-mm-dd or as dd-mmm-yyyy to prevent day/month confusion as to the locale, these always work (obviously) while dd-mm is always prone to confusion between the US and Europe. But this is your project, I'll proceed as you prefer

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am EU based, so I prefer the dd-mm-yyyy HH:MM myself as a default.
If this becomes an issue later on, then i think it would be easy to add an user specified option to change the date formatting. But I think it is fine to leave it as is.

The suggested change only removes the 3'rd m in the month of the format

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also changing the comment won't change the code which is hard coded to provide this format. I'm no lua expert, you might have a better way to do that, e.g. we could put that in the options and let the user choose its date format and if lua has a date/time formatting function we can rely on we can use it. Wdyt?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not aware that you can get the date format from lua itself, so the best option is to let the user choose the date format, and provide the default as dd-mm-yyyy HH:MM

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll try to figure out a better way to work on formatting dates and giving the user reasonable options.

plugin/init.lua Outdated
local hour = stdout:sub(9, 10)
local minute = stdout:sub(11, 12)

-- Format into "dd-mmm-yyyy hh:mm"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-- Format into "dd-mmm-yyyy hh:mm"
-- Format into "dd-mm-yyyy hh:mm"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

ChrisGVE and others added 4 commits December 31, 2024 10:30
Co-authored-by: Malthe Larsen <[email protected]>
Co-authored-by: Malthe Larsen <[email protected]>
Co-authored-by: Malthe Larsen <[email protected]>
Co-authored-by: Malthe Larsen <[email protected]>
@ChrisGVE
Copy link
Author

To answer your question: I have not noticed a difference, but I don't have that many states stored so the approach might be suboptimal, e.g. to be fast we could try to retrieve everything in one go rather than picking up the glob and then enriching it with the date/time for each file. But I wasn't sure how to achieve it.

Second, yes we can make it an option same as the date format. But I will need some time to achieve both.

Btw I am giving a try at Ghostty, and thus back with tmux 😉. Let's see how it shapes up.

ChrisGVE and others added 4 commits December 31, 2024 10:59
Co-authored-by: Malthe Larsen <[email protected]>
- the function no longer rely on wezterm.glob, instead it runs shell
  commands for Mac, Linux, and Windows (powershell is used with
  windows).
- the function parses the result into a table which is further processed
  for formating, including the date of the last change of the state.
- the options include the following new key-values
  - `show_date_with_state`, a boolean which indicates whether the date
    is shown in the fuzzy list. Default is false.
  - `date_format`, a strftime-string which gives the format to be used
    when displaying the date. Default is "%d-%m-%Y %H:%M:%S"
@ChrisGVE
Copy link
Author

I've refactored the whole code. It should now be faster as all the data is fetched by the OS, instead of relying on wezterm API and returning fetching the rest one by one. The cost is that there is now one command for Mac, one for Linux, and one for Windows. Unfortunately, I did not have time to check them besides the Mac one, which works well.

There are now two new key-values in the opts for the fuzzy_load

  • show_state_with_date, default false, which can be used to add the date (works for both read and delete)
  • date_format, default "%d-%m-%Y %H:%M:%S", a strftime-string to format the date and can be selected by the user.

The documentation has also been updated to factor in the changes in this PR.

Do you have a chance to test the Linux and Windows versions yourself? Unfortunately, I won't have the time in the coming weeks. I can only handle your guidance, feedback, and change requests. I'm very sorry about that.

Copy link
Owner

@MLFlexer MLFlexer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, awesome that you could improve the performance with a single popen call instead!

I have added some minor changes I would like to see changed 😄
I will try and test it out on linux and windows in the near future.

Great work!

@ChrisGVE
Copy link
Author

ChrisGVE commented Jan 5, 2025

I’ve fixed the code as the displayed choices were including the full filepath of the saved state, now it only gives the filename.

BTW I’m back using wezTerm, Ghostty does not fit my needs right now.

@ChrisGVE ChrisGVE requested a review from MLFlexer January 5, 2025 09:44
@MLFlexer
Copy link
Owner

MLFlexer commented Jan 23, 2025

Sorry for the late reply, I had to focus on exams the last 2 months

I tried to test it on Windows, and there are 2 things that I initially notices:

  1. On windows, when you start subprocesses with the lua io api, it seems to open cmd gui windows which run the subprocesses, super weird behaviour, but i guess that is windows for ya...
    To combat this we should use the run_child_process.

  2. There is no input to the picker. I tried to print the command being run and ran it myself locally, here is the output:

powershell -Command "Get-ChildItem -Path 'C:\Users\Admin\Desktop\state\window' | ForEach-Object { "$($_.LastWriteTime.ToFileTimeUtc()) $($_.Name)" }"
You cannot call a method on a null-valued expression.
At line:1 char:102
+ ... indow' | ForEach-Object { "$($_.LastWriteTime.ToFileTimeUtc()) $($_.N ...
+                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (:) [], RuntimeException
    + FullyQualifiedErrorId : InvokeMethodOnNull

I am by no means a windows user other than for gaming, so I don't know the best way to do this.
One option would be to merge this PR without windows support and then create a seperate issue for adding window support? This might be the best way to move foreward, if windows testing is an issue? Let me know what you think 😃

@ChrisGVE
Copy link
Author

No worries. I am busy with work and personal projects as well.

Also, there are a few things I need to check regarding the use of your plugin that doesn't work well with me (probably a configuration error).

In any case, I plan to look at Windows and Linux and see what works and what does not. I have them as virtual machines. It is only a matter of time.

With respect to the PR, I think the project will benefit more if things are working correctly on all target OSes. Now, it's your baby, so I'll follow your lead, but that's my two cents.

@MLFlexer
Copy link
Owner

No worries. I am busy with work and personal projects as well.

Also, there are a few things I need to check regarding the use of your plugin that doesn't work well with me (probably a configuration error).

In any case, I plan to look at Windows and Linux and see what works and what does not. I have them as virtual machines. It is only a matter of time.

With respect to the PR, I think the project will benefit more if things are working correctly on all target OSes. Now, it's your baby, so I'll follow your lead, but that's my two cents.

I agree, it is best to have all versions working. So if you are able to make working versions for windows, then it would be awesome! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants