Skip to content

Adding Setting to control palette preview editor's width#895

Merged
dzhou121 merged 9 commits intolapce:masterfrom
jmj0502:issue-872
Aug 6, 2022
Merged

Adding Setting to control palette preview editor's width#895
dzhou121 merged 9 commits intolapce:masterfrom
jmj0502:issue-872

Conversation

@jmj0502
Copy link
Copy Markdown
Contributor

@jmj0502 jmj0502 commented Jul 29, 2022

Closes #872

  • The preview_editor_width an its respective getter function were added to the UIConfig struct on lapce-data/config.rs.
  • A default value preview_editor_width was added to defaults/settings.toml.
  • The variable input_width was added to the layout method on pallete.rs, it's value will be the difference between the input_size.width and width, if the value of data.config.ui.preview_editor_width is greater than the value contained on width, or zero. Such variable is used as helper to center the input bar appropriately based on the preview_editor_width.
  • The width of the different elements defined on the layout function is now computed using f64::max in order to keep consistency on the UI.

@MinusGix
Copy link
Copy Markdown
Member

You need to fix the rustfmt of the files. (I'd also suggest making it a single commit)

That method does work, though I think it would be better to split the palette's width from the preview editor's width. Being able to set the width of the preview separate from the width of the palette would be nice (as typically the palette entries are small).

@jmj0502
Copy link
Copy Markdown
Contributor Author

jmj0502 commented Jul 29, 2022

@MinusGix Okok, I see. That solution is cool and I think that I can still jump back to such approach. However, the UI looked like if it was broken in some cases; that's the only reason why I took another approach (any suggestions in regards to this will be helpful) 🤕.

@MinusGix
Copy link
Copy Markdown
Member

To just do the preview-width editing, you can revert the size changing for the content rect and input box, and also the set origins.
I tried that locally and it seemed to work, the only problem seemed to be centering the preview editor.
If there's a specific UI bug you run into, then you could upload the code causing it to the PR and I can take a look at it.

@MinusGix MinusGix added the A-ui Area: UI rendering and interactions label Jul 29, 2022
@jmj0502
Copy link
Copy Markdown
Contributor Author

jmj0502 commented Jul 30, 2022

@MinusGix I reverted to the original implementation. I also sneaked in a little logic from the second implementation, in order to center the editor. I was able to come up with this (the preview_editor_width is set to 600px in the example):
image
Also, The preview editor width is now independent from the palette width; and the content widget resizes with it. However, the preview editor gets kind of broken at 700px (line numbers disappear 🤕). Do you have any idea of what might be happening?

@MinusGix
Copy link
Copy Markdown
Member

MinusGix commented Jul 30, 2022

I was able to get this
image
by

ctx.set_paint_insets(4000.0);

let self_size = Size::new(width, input_size.height + content_height + preview_height);
self.content_rect = Size::new(width, self_size.height)
            .to_rect()
            .with_origin(Point::new(0.0, 0.0));

I did see the bug where the line-numbers disappear, when I didn't have the paint_insets
I think Druid doesn't really like the negative points (which you use for centering, which was a good idea) when they escape a certain bounding area that it expects you to paint in, and so I think we have two possible solutions:

  • Try to reposition the base drawing x-position to be where the palette preview editor's x-position is (aka, wherever calls set_origin on the Palette). Then, rather than shifting the preview editor, we shift the palette input. Basically just starting at the far left side. However this requires it to know what the appropriate widths is, which might be a bit rough?
  • Then there's the solution of just telling Druid that we want to allow 'overpainting'. We already do this for the palette, so doing it again is probably fine and the easiest solution. ctx.set_paint_insets(4000.0); after all the laying out in the layout function, like in my example code above.

@jmj0502
Copy link
Copy Markdown
Contributor Author

jmj0502 commented Jul 30, 2022

@MinusGix Ok, cool. I'll try the second option first (since that seems to be easier and the quicker to apply). If that doesn't work, I'll proceed to ask more details about the first one 👍

…order allow overpainting, so the preview editor can be centered. The default value of the preview_editor_width was updated to 600px.
@jmj0502
Copy link
Copy Markdown
Contributor Author

jmj0502 commented Jul 30, 2022

@MinusGix I think its done! I added the line you suggested while keeping the negative x coordinate, in order to center the editor (worked pretty well). I try the changes using different widths (600, 700, 800, 900 and 1000px) and the preview editor looks good on all of them ✌️. I also set the default width to 600px, since that will allow the users to notice a little difference between the palette width and the preview editor width.

@MinusGix
Copy link
Copy Markdown
Member

Nice work. Thanks!
Minor: I forgot to comment on this before, but in Rust, typically variables with an _ before their name are 'unused'. So you should rename _preview_size to preview_size since you're not using it.

Could you also 'collapse' the PR into a single commit?

@jmj0502
Copy link
Copy Markdown
Contributor Author

jmj0502 commented Jul 31, 2022

@MinusGix Thanks for all the guidance! No problem, I'll make the change!
I haven't collapsed commits manually (always use the GitHub option while dealing with PRs), but I'll give it a shot!

@jmj0502
Copy link
Copy Markdown
Contributor Author

jmj0502 commented Jul 31, 2022

@MinusGix Can you give me a hand with the squash part? (tried a rebase locally but didn't seem to work) 😭
A squash and merge should do the trick! (In case you can, here's the message: Adding a new UI setting that will allow users to modify the palette preview editor's width.)

@MinusGix
Copy link
Copy Markdown
Member

It's fine :)
@dzhou121 can select the squash-and-merge option when he merges the PR. (He's the one that does the merging)

@jmj0502
Copy link
Copy Markdown
Contributor Author

jmj0502 commented Aug 6, 2022

@MinusGix Hey! Hope you're doing great!
Question. Will this PR land? 😢

@panekj
Copy link
Copy Markdown
Collaborator

panekj commented Aug 6, 2022

Will this PR land? 😢

It depends on @dzhou121

@dzhou121 dzhou121 merged commit 7b5e19e into lapce:master Aug 6, 2022
panekj pushed a commit to panekj/lapce that referenced this pull request Aug 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ui Area: UI rendering and interactions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setting to control palette preview editor's width

4 participants