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

Add User experience menu #769

Merged
merged 1 commit into from Aug 1, 2022
Merged

Add User experience menu #769

merged 1 commit into from Aug 1, 2022

Conversation

osamuaoki
Copy link
Contributor

Building on discussion and tutoring at #764

I refactored original code set

Signed-off-by: Osamu Aoki osamu@debian.org

@github-actions
Copy link

Use the following updater settings in your user/init.lua file, restart, and run :AstroUpdate to test this pull request:

  updater = {
    channel = "nightly",
    branch = "ux",
    remote = "osamuaoki",
    remotes = {
      ["osamuaoki"] = "osamuaoki/AstroNvim",
    },
  },

@osamuaoki
Copy link
Contributor Author

I realized my function definition style was not in line with other parts. I will address it later.

@osamuaoki
Copy link
Contributor Author

  • I only changed upper case usage style for" URL highligts" but wondering I should change to "URL highlight" without "s".
  • I almost added "toggle paste mode" but since it is deprecated in nvim, I didn't.

Copy link
Member

@mehalter mehalter left a comment

Choose a reason for hiding this comment

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

I made some comments with some optimizations/simplifications of the code. Also could we replace all of the print calls with astronvim.notify calls

Comment on lines 82 to 95
local relativenumber = vim.wo.relativenumber -- local to window
if (number == false) and (relativenumber == false) then
vim.wo.number = true
vim.wo.relativenumber = false
elseif (number == true) and (relativenumber == false) then
vim.wo.number = false
vim.wo.relativenumber = true
elseif (number == false) and (relativenumber == true) then
vim.wo.number = true
vim.wo.relativenumber = true
else -- (number == true) and (relativenumber == true) then
vim.wo.number = false
vim.wo.relativenumber = false
end
Copy link
Member

Choose a reason for hiding this comment

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

could we optimize and simplify this a little bit and do

  if not number and not relativenumber then
    vim.wo.number = true
  elseif number and not relativenumber then
    vim.wo.relativenumber = true
  elseif number and relativenumber then
    vim.wo.number = false
  elseif not number and relativenumber then
    vim.wo.relativenumber = false
  end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Have you pushed this change? I don't see it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I moved it to ui.lua

Copy link
Member

Choose a reason for hiding this comment

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

I meant the rewrite I recommended for the number setting cycling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: I pushed to my ux branch which shows up on you account as osamu_ux
You may need to explicitly pull osamu_ux.

-- Change the number display modes.
function astronvim.change_number()
  local number = vim.wo.number          -- local to window
  local relativenumber = vim.wo.relativenumber  -- local to window
  if not number and not relativenumber then
    vim.wo.number = true
    vim.wo.relativenumber = false
  elseif number and not relativenumber then
    vim.wo.number = false
    vim.wo.relativenumber = true
  elseif not number and relativenumber then
    vim.wo.number = true
    vim.wo.relativenumber = true
  else -- number and relativenumber
    vim.wo.number = false
    vim.wo.relativenumber = false
  end
  vim.notify(
    string.format("number=%s, relativenumber=%s",
      bool2str(vim.wo.number),
      bool2str(vim.wo.relativenumber)
    )
  )
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha,…I see your point sir.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done this time. Thanks

lua/configs/which-key-register.lua Outdated Show resolved Hide resolved
lua/core/utils/init.lua Outdated Show resolved Hide resolved
lua/core/utils/ux.lua Outdated Show resolved Hide resolved
@pkazmier
Copy link
Contributor

What about adding a toggle wrap? Sometimes when editing markdown files, I notice the author hard wraps, other times they don’t. Would be nice to have a quick toggle. Thoughts?

Copy link
Member

@mehalter mehalter left a comment

Choose a reason for hiding this comment

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

I noticed you changed the prints to astronvim.echo could you change them to astronvim.notify?

@osamuaoki
Copy link
Contributor Author

I noticed you changed the prints to astronvim.echo could you change them to astronvim.notify?
Woops. s/echo/a...notify/

@osamuaoki
Copy link
Contributor Author

What about adding a toggle wrap? Sometimes when editing markdown files, I notice the author hard wraps, other times they don’t. Would be nice to have a quick toggle. Thoughts?

Added ... (We can remove this if there is a objection.)

@osamuaoki
Copy link
Contributor Author

I enhanced this for nvim-autopairs.

@osamuaoki
Copy link
Contributor Author

I think this on-the-fly UI behavior change capability is getting matured now.

One shortcoming is if filetype plugins or something else sets indent settings, vales set by <leader>ui is ignored. This happens with files like the lua source ones. Considering this feature is some file not well supported by FTP such as plain text file, this is OK for now. Any improvement idea is welcome.

Initial starting UI behavior customization should be a part of user_examples, I added them but I am 2 missing.

  • vim.g.autopairs_toggle_flag
  • vim.g.cmp_toggle_flag
    If anyone knows best way, let me know.

@osamuaoki
Copy link
Contributor Author

toggling for CMP and autopair needs some work. (Otherwise initial mode setting doesn't worrk and it always start active.)

@osamuaoki
Copy link
Contributor Author

I think I need to work more on autopairs and cmp. Excuse me.

@osamuaoki
Copy link
Contributor Author

I rebased to the current main.

DIFF: https://github.com/AstroNvim/AstroNvim/compare/main..e7aefed4a0463b08df5f55de4c9beb362a2af8ea

I think this is the best effort for me.

I could not figure out how to start autopairs with disabled state cleanly.
So it starts as ON which is current AstroNvim behavior. (I tried autcmd etc...)
For cmp, I changed toggling method.

Thanks for interesting code base to play with.

osamu

@mehalter
Copy link
Member

Awesome, thanks so much @osamuaoki ! This is a great contribution and you have done a LOT of heavy lifting to get this PR to where it is :) I will take a look at what's currently there and merge it into a development branch to finish it up and get it merged. Thanks again :D

@osamuaoki
Copy link
Contributor Author

Thanks.

I think rebase conflict is minimal one since we both changed the last line of core/utils/init.lua. Please make both changes accepted.

One thing I am concerned mildly is use of file name ui.lua.

Your development branch seems to have core/ui.lua. I don't think this cause any real problem with core/utils/ui.lua in my code for lua-JIT but having the same file name may be confusing for humans.

@osamuaoki
Copy link
Contributor Author

I finally figured out how to start autopairs in disabled state. It was packer setup .

Unlike, cmp, where we need a vim.g variable to toggle it, autopairs has internal state variable. So in theory, we don't need to create vim.g variable and have fancy user configuration trick.

But that will be mind twisting to follow or write. So I chose simple code.

So I can make all CMP/autopairs as opt-in feature. I like them when I write first code but for debugging, it is annoying.

(I was not able to get v2 running cleanly. Do I need nvim 0.8 pre-release?)

@mehalter
Copy link
Member

Oh yeah @osamuaoki currently v2 does require neovim 0.8 nightly. Sorry about that!

@osamuaoki
Copy link
Contributor Author

I noticed you changed the prints to astronvim.echo could you change them to astronvim.notify?

Done: This has been implemented.

I will squash changes and clean up ...

@osamuaoki osamuaoki requested a review from mehalter July 28, 2022 04:45
Copy link
Member

@mehalter mehalter left a comment

Choose a reason for hiding this comment

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

@osamuaoki looks amazing! Thanks so much! Last thing though: could you reword your commit to say: feat(mappings)!: add UI menu feature

It's more of a mappings feature than a which-key one, also it does have a breaking change, so it needs the !

Several new mappings are introduced to control AstroNvim UI features.
They are accessible from Normal mode under the "+UI" which-key menu.

  * Toggle autopairs                          (if module is available)
  * Toggle background
  * Toggle completion                         (if module is available)
  * Toggle signcolumn
  * Change indent setting
  * Change line numbering
  * Toggle spellcheck
  * Toggle URL highlight                     (relocated menu position)
  * Toggle wrap
  * Toggle syntax highlight

For URL highlight, autopairs (nvim-autopairs) and completion (nvim-cmp),
their starting states can be controlled by setting following vim global
variables:

  * vim.g.highlighturl_enabled            (no change with this commit)
  * vim.g.autopairs_enabled             (new feature with this commit)
  * vim.g.cmp_enabled                   (new feature with this commit)
  * vim.g.syntax_on           (`:syntax on` or `:syntax off` commands)

For other vim native features, their corresponding starting states can
be controlled by setting following standard vim options:

  * background
  * signcolumn
  * expandtab
  * tabstop
  * softtabstop
  * softwidth
  * number
  * relativenumber
  * spell
  * wrap
  * syntax

The syntax option and toggling of it also affects treesetter.

Signed-off-by: Osamu Aoki <osamu@debian.org>
@osamuaoki
Copy link
Contributor Author

  • I have updated commit message according to the request.
  • I rebased this to the latest main.
  • I added if_available to 2 mappings since user may disable the corresponding packages. (I think this is right way. Please double check modules.lua file.

Please note I have corresponding PR for web page. https://github.com/AstroNvim/astronvim.github.io/pull/13

In this PR, I also add "Configuration Mechanism" section so people can configure this package without reading the Lua source code around user_plugin_opts. (Intentionally terse.)

@osamuaoki osamuaoki requested a review from mehalter August 1, 2022 04:34
@mehalter mehalter changed the base branch from main to v2 August 1, 2022 11:47
@mehalter mehalter changed the base branch from v2 to ui_dev August 1, 2022 11:50
Copy link
Member

@mehalter mehalter left a comment

Choose a reason for hiding this comment

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

LGTM! I am going to merge this into a dev branch and do a quick rebase on the v2 branch and merge it there since it has breaking changes. Thanks so so so much for your hard work on this one!

@mehalter mehalter merged commit f183edd into AstroNvim:ui_dev Aug 1, 2022
@mehalter
Copy link
Member

mehalter commented Aug 1, 2022

Finishing up at #816

@osamuaoki
Copy link
Contributor Author

Thanks.

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.

None yet

3 participants