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

CoqIDE: Improve find/replace UI (margins, icons) #18523

Merged
merged 1 commit into from Feb 23, 2024

Conversation

Frigory33
Copy link
Contributor

@Frigory33 Frigory33 commented Jan 21, 2024

Fixes #11024

  • Added changelog.

@Frigory33 Frigory33 requested a review from a team as a code owner January 21, 2024 11:41
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jan 21, 2024
ide/coqide/wg_Find.ml Outdated Show resolved Hide resolved
@Frigory33 Frigory33 force-pushed the coqide-findreplace branch 2 times, most recently from dc85911 to 2808666 Compare January 25, 2024 18:58
@jfehrle
Copy link
Member

jfehrle commented Feb 19, 2024

These are the differences I see between master and this PR. Perhaps you want to add a little vertical space between the Find and "Replace with" lines and below "Replace with"?

image

image

FWIW the Find/Can't find icons don't show up until you've done a search or replace:

image

image

Copy link
Member

@jfehrle jfehrle left a comment

Choose a reason for hiding this comment

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

The changes here are minor. I think they won't make any significant difference in usability for users. Given that we have limited resources to make and review changes and the many things that deserve to be improved, I hope you'll focus on more impactful changes in the future.

@@ -137,13 +139,16 @@ class finder name (view : GText.view) =
end

method private set_not_found () =
find_entry#misc#modify_bg [`NORMAL, `NAME "#F7E6E6"];
Copy link
Member

Choose a reason for hiding this comment

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

Adding the icon and the tooltip are fine, but I would keep the reddish/pink background after an unsuccessful find because some users may prefer the current behavior. GUI detail preferences are subjective.

Copy link
Contributor Author

@Frigory33 Frigory33 Feb 20, 2024

Choose a reason for hiding this comment

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

The part of it which is not subjective is bug #11024 which was caused by the background color. This is the reason to use icons instead.


method private set_found () =
find_entry#misc#modify_bg [`NORMAL, `NAME "#BAF9CE"]
Copy link
Member

Choose a reason for hiding this comment

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

Same thing, I would keep the green.

Comment on lines -27 to -30
let _ = GMisc.label ~text:"Find:" ~xalign:1.0
~packing:(replace_box#attach
(*~xpadding:3 ~ypadding:3*) ~left:0 ~top:1 (*~fill:`X*)) () in
let _ = GMisc.label ~text:"Replace:" ~xalign:1.0
Copy link
Member

Choose a reason for hiding this comment

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

The xaligns are not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because I’m using set_halign below, which looks more relevant. If using ~xalign:1.0 instead, labels are aligned to the right. If using none of them, labels are centered. According to https://docs.gtk.org/gtk3/property.Label.xalign.html xalign should correspond to a “labels size allocation,” but well, how it works is less clear to me than halign.

@Frigory33
Copy link
Contributor Author

With my GTK theme, the vertical spacing was quite okay:
image
If the Detach button on the left is not taller than the grid of widgets, indeed, it lacks space. Let’s add some.

@Frigory33
Copy link
Contributor Author

I added row and col spacing in the grid, and margin around the grid:
image

Copy link
Member

@jfehrle jfehrle left a comment

Choose a reason for hiding this comment

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

LGTM.

If there is no objection, I will merge soon.

@jfehrle jfehrle removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Feb 23, 2024
@jfehrle jfehrle self-assigned this Feb 23, 2024
@jfehrle jfehrle added this to the 8.20+rc1 milestone Feb 23, 2024
@jfehrle
Copy link
Member

jfehrle commented Feb 23, 2024

@coqbot: run full ci

@jfehrle jfehrle added the part: CoqIDE Issues and PRs related to CoqIDE or other IDE features of coq. label Feb 23, 2024
@jfehrle
Copy link
Member

jfehrle commented Feb 23, 2024

@coqbot: merge now

Copy link
Contributor

coqbot-app bot commented Feb 23, 2024

@jfehrle: You cannot merge this PR because:

  • There is no kind: label.

@jfehrle jfehrle added the kind: usability Not a bug (fix) but a usability issue / improvement, e.g. UI or syntax inconsistencies label Feb 23, 2024
@jfehrle
Copy link
Member

jfehrle commented Feb 23, 2024

@coqbot: merge now

@coqbot-app coqbot-app bot merged commit 659e848 into coq:master Feb 23, 2024
5 of 6 checks passed
@Frigory33 Frigory33 deleted the coqide-findreplace branch February 23, 2024 21:11
@herbelin
Copy link
Member

By the way, would it be easy to add a way to remove the search/replace window? Currently, the way I use is to detach and delete the detached window, but a one-click way would be convenient.

@Frigory33
Copy link
Contributor Author

By the way, would it be easy to add a way to remove the search/replace window?

Just press Escape.

@herbelin
Copy link
Member

Just press Escape.

Wonderful, I learn new things every day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: usability Not a bug (fix) but a usability issue / improvement, e.g. UI or syntax inconsistencies part: CoqIDE Issues and PRs related to CoqIDE or other IDE features of coq.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selecting text in Search box in CoqIDE
4 participants