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

Prompt buffer mark checkboxes #3176

Merged
merged 3 commits into from
Dec 7, 2023
Merged

Conversation

aartaka
Copy link
Contributor

@aartaka aartaka commented Sep 19, 2023

Description

This adds a checkbox to every mark-able suggestion in prompt buffer. Looks like this right now:

marking-checkboxes

Clicking on a checkbox scrolls the prompt one suggestion down (like it does with C-space/M-space marking), which is convenient. I've also restricted the area where clicks mark/return suggestions to the attribute values they list.

Note that I didn't apply the styling yet, because I was mainly working with the structure of the prompt in general. One particular thing that diverges from @lansingthomas' spec is the "Mark" column. I had to add it because otherwise prompt becomes much less structured and breaks table markup in general :P @lansingthomas, is the structure okay? Good to proceed with styling?

Fixes #3134

Discussion

Mention there any suspicious parts of the new code, or the ideas that you'd like to discuss in regards to this change.

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

  • Git hygiene:
    • I have pulled from master before submitting this PR
    • There are no merge conflicts.
  • I've added the new dependencies as:
    • ASDF dependencies,
    • Git submodules,
      cd /path/to/nyxt/checkout
      git submodule add https://gitlab.common-lisp.net/nyxt/py-configparser _build/py-configparser
    • and Guix dependencies.
  • My code follows the style guidelines for Common Lisp code. See:
  • I have performed a self-review of my own code.
  • My code has been reviewed by at least one peer. (The peer review to approve a PR counts. The reviewer must download and test the code.)
  • Documentation:
    • All my code has docstrings and :documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)
    • I have updated the existing documentation to match my changes.
    • I have commented my code in hard-to-understand areas.
    • I have updated the changelog.lisp with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).
      • Changelog update should be a separate commit.
    • I have added a migration.lisp entry for all compatibility-breaking changes.
    • (If this changes something about the features showcased on Nyxt website) I have these changes described in the new/existing article at Nyxt website or will notify one of maintainters to do so.
  • Compilation and tests:
    • My changes generate no new warnings.
    • I have added tests that prove my fix is effective or that my feature works. (If possible.)
    • I ran the tests locally ((asdf:test-system :nyxt) and (asdf:test-system :nyxt/gi-gtk)) and they pass.

@lansingthomas
Copy link
Contributor

Nice work,

:P @lansingthomas, is the structure okay? Good to proceed with styling?

Almost - lets try a few things first. See you tomorrow.

@jmercouris
Copy link
Member

Result of meeting last week? What must we do?

@lansingthomas
Copy link
Contributor

Result of meeting last week? What must we do?

It was decided that Artyom will make the extra column disappear 🫥 per the spec in #3134

@lansingthomas
Copy link
Contributor

we talked through what other options might look like and they don't look great.

@aadcg
Copy link
Member

aadcg commented Oct 18, 2023

What's the status on this one? @aartaka @lansingthomas

@aartaka
Copy link
Contributor Author

aartaka commented Oct 18, 2023 via email

@lansingthomas
Copy link
Contributor

@aartaka I am all good on checkboxes. Sorry for any confusion.

@aadcg aadcg self-requested a review October 20, 2023 07:04
aadcg
aadcg previously requested changes Oct 23, 2023
Copy link
Member

@aadcg aadcg left a comment

Choose a reason for hiding this comment

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

This PR is manifestly not ready. See the bug below - the user is unable to unmark the top suggestion.

simplescreenrecorder-2023-10-23_10.23.53.mp4

Requesting peer reviews for unfinished or not self-reviewed work is to be avoided at all costs.

@aadcg aadcg marked this pull request as draft October 23, 2023 07:28
@jmercouris
Copy link
Member

@aartaka OK for me to take over this one?

@aartaka
Copy link
Contributor Author

aartaka commented Nov 8, 2023 via email

@aartaka
Copy link
Contributor Author

aartaka commented Nov 8, 2023 via email

@jmercouris jmercouris self-assigned this Nov 9, 2023
@jmercouris
Copy link
Member

I finally got Nyxt compiling against this branch. I rm'd NASDF from all subdirectories on my machine, and it started working. I can start working on this again.

@jmercouris
Copy link
Member

I've not been able to solve the bug yet. I have been working on this, though.

@jmercouris
Copy link
Member

It however doesn't happen only with the top checkbox. I have been able to get it to happen with other checkboxes.

@jmercouris
Copy link
Member

@aadcg I believe I have fixed the bug. I have done some testing, and have simplified a lot of the logic. Please give it a try now.

@aadcg
Copy link
Member

aadcg commented Dec 2, 2023

@jmercouris when requesting a review, please ensure that the branch is rebased over master. Do you see the pertinence of this request?

@jmercouris
Copy link
Member

Sure, but I am not saying it is completely ready, I am just saying that I believe I fixed the bug :-D

I can rebase over master next time as well!

@jmercouris
Copy link
Member

Well.. I see now that I /was/ requesting a review. But! I thought there was still work to be done, not sure if it is to be called "Mark" or whatever. I think there were some stylistic changes to make!

@jmercouris jmercouris force-pushed the prompt-buffer-mark-checkboxes branch from b81b497 to 72a134b Compare December 2, 2023 19:09
@aadcg aadcg dismissed their stale review December 2, 2023 21:24

pr status changed in the mean time.

@aadcg
Copy link
Member

aadcg commented Dec 2, 2023

@jmercouris, I see the misunderstanding! I'd suggest completing the PR, including all of the final details, and requesting a review at the very end when you feel it's ready to be merged. "Intermediary" reviews may be needed at times (though they should be avoided for the sake of efficiency), but perhaps not in this case.

@jmercouris
Copy link
Member

I've tweaked the visual appearance closer to the specification:

Screenshot_20231204_150432

What do you say @lansingthomas ?

@jmercouris jmercouris force-pushed the prompt-buffer-mark-checkboxes branch from c0b07fd to 412dff0 Compare December 4, 2023 21:18
@jmercouris
Copy link
Member

Screenshot_20231204_151910

Lastly, added a little bit of padding.

@jmercouris jmercouris marked this pull request as ready for review December 4, 2023 21:20
@jmercouris
Copy link
Member

@aadcg R E A D Y!

@lansingthomas
Copy link
Contributor

Looks great. Don't let styling get in your way at this point. We just need to help people intuit this whole modal experience.

@aadcg
Copy link
Member

aadcg commented Dec 5, 2023

@jmercouris we'll discuss it on the call. Thanks.

@aadcg aadcg force-pushed the prompt-buffer-mark-checkboxes branch from 412dff0 to e720b05 Compare December 5, 2023 16:43
@jmercouris jmercouris force-pushed the prompt-buffer-mark-checkboxes branch from e720b05 to 75bfe70 Compare December 7, 2023 19:11
@jmercouris jmercouris merged commit 9bd4e9b into master Dec 7, 2023
2 checks passed
@jmercouris jmercouris deleted the prompt-buffer-mark-checkboxes branch December 7, 2023 19:19
@aadcg
Copy link
Member

aadcg commented Dec 7, 2023

Thanks!

@jmercouris
Copy link
Member

I am finally a merging magician!

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

Successfully merging this pull request may close these issues.

affordances for the toggle-modes menu (checkboxes)
4 participants