Skip to content

Conversation

@Wittmaxi
Copy link
Owner

No description provided.

@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch 4 times, most recently from 9298054 to 38db5ef Compare October 2, 2023 08:08
@HeikoKlare
Copy link

I have taken an initial look at the functionality. Overall, it already looks quite nice!
Still I have some initial findings. I will have another look once the essential bugs are fixed. I would propose to address the "unexpected behaviors" and the "todos for now" before asking the community for feedback, so that they can focus on whether the dialog behaves as expected from a user/UX perspective.

Unexpected Behaviors

  • Pressing ENTER in the replace field does only perform a search but no replacement. I would expect it to replace the current selection and not require ALT+ENTER to be pressed.
  • When the dialog is open but focus is in the text editor, pressing CTRL+F will not move focus to the dialog but will close it. I would expect focus to move to the dialog.
  • When closing the dialog via ESCAPE, it will require CTRL+F to pressed twice to open it again (because of the previous point).
  • I can create multiple replace inputs. This can be achieved by pressing TAB and then ENTER from within the replace input box (even multiple times):
    image
    It then takes the input in the last opened text field for replacement, even when clicking the replace button in some other row.

Todos For Now

  • We should provide a button to switch to the pre-existing dialog. This is particularly important for the PoC because it eases comparison of old and new behavior, as you can easily switch between those functionalities.
  • May also for later, but could also be great for PoC and/or demo at EclipseCon: Dialog is only shown in the currently active view but should shown until it is closed.

Todos For Later

They should at least be documented as things to still be addressed in the feature development when starting a PoC PR:

  • Replace/Find function should be provided in some way (i.e., the next find result is selected after performing a replacement).
  • We could think about having a possibility to close the dialog not only via keyboard but also via a close button ("X") or the like.
  • My feeling is that the UI will look cleaner if the two text boxes have equal width even if a different number of buttons is placed next to them.
  • Resizing is kind of broken at some point when the dialog becomes too small:
    image
  • When the width of the dialog is changed while the containing view is too small to contain the "fully-sized" dialog, the graphics canvas is not properly flushed so that artifacts from the previous size remain after repaining. It is hard to make a screenshot (I can take a video if necessary), but is easy to reproduce when making the containing view quite small.

@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch from 38db5ef to 334b8d6 Compare October 2, 2023 14:55
@HeikoKlare
Copy link

From my point of view, the state it great for presenting it to the community. There is only one little thing I would propose to address before submitting a draft PR to discuss the implementation.

  • When the overlay is closed while the "replace" part is open, then the overlay is opened again (via CTRL+F), then the "replace" part will not open upon clicking "+". You need to click expand, collapse, expand until it opens. This is probably related to some old state of the overlay still being present.

And there are two minor additions that you should put on the list to be tracked to be addressed later (and mentioned in the initial PR):

  • I would expect pressing ESCAPE within the editor (not the overlay) to also close the overlay. But that's something I am absolutely open for discussion.
  • When you do a search and then click "replace" multiple times, you have to undo as many times to go back to the original content. I would expect a "replace" to only be applied (and written to operation history) only once.

@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch 4 times, most recently from b5cd48f to 659b102 Compare October 5, 2023 10:52
@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch from 659b102 to 0455efe Compare November 7, 2023 11:12
@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch 2 times, most recently from fae805d to f975bc4 Compare December 8, 2023 14:53
@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch from f975bc4 to 0e9bd7c Compare January 13, 2024 14:11
@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch 9 times, most recently from 8910b98 to 2481a86 Compare January 30, 2024 15:59
@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch 6 times, most recently from 8a2fafd to 0e22e4b Compare February 10, 2024 10:01
@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch 2 times, most recently from df51067 to 8ad526d Compare June 5, 2024 20:44
The API filter added for the AbstractTreeViewer for the 4.32 release
became obsolete when moving to the 4.33 stream. This change removes the
now obsolete filter.

See eclipse-platform#1072
@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch 3 times, most recently from 808bea1 to 4f855bb Compare June 6, 2024 07:28
Maximilian Wittmer and others added 2 commits June 6, 2024 10:28
Removes the beep which is issued after some unsuccessful operations in
the find/replace dialog.

fixes eclipse-platform#1908
@HeikoKlare HeikoKlare force-pushed the MW_FindReplace_Overlay branch from 3b16b9e to 1951145 Compare June 6, 2024 09:07
jukzi and others added 6 commits June 6, 2024 11:49
'The method newInstance() from the type Class<capture#3-of ?> is
deprecated since version 9'

tested by
org.eclipse.e4.ui.tests.css.core.parser.ValueTest
…eclipse-platform#1895

This reverts commit 8abe1b1.

The PreferenceDialog has a custom size calculation that tries to adapt
to the current page. That contradicts using the the former size

eclipse-platform#1895
The `IFileInfo.toString()` isn't intended to be shown to user, it has no
information interesting for users.

The `IFileStore.toString()` should be used in cases if information about
deleted resources need to be shown in UI.

Additionally changed dialog wording to not use "location" and be more
clear about what's going on.

Fixes eclipse-platform/eclipse.platform#1404
@HeikoKlare HeikoKlare force-pushed the MW_FindReplace_Overlay branch from 1951145 to dac9ac8 Compare June 6, 2024 13:04
BeckerWdf and others added 6 commits June 6, 2024 16:12
When registering a link handler on macOS the info.plist file of the
eclipse application needs to be changed. If the eclipse application is
signed this breaks the signature of the eclipse application. Breaking
the signature has the effect that the eclipse application cannot be
started any more (at least after an reboot of the operating system).

This means we cannot register any additional link handlers at runtime
for signed macOS applications. Changes the behavior for singed
macOS applications to the following:

- Don't run automatic registration at startup of the application
- Disable the "Apply" function on the "Link Handlers" preference page
- Show error message on the "Link Handlers" preference page
'The method isAccessible() from the type AccessibleObject is deprecated
since version 9'

tested with
 WorkbenchThemeChangedHandlerTest,
 org.eclipse.ui.tests.commands.HelpContextIdTest.testHelpContextId()
@Wittmaxi Wittmaxi force-pushed the MW_FindReplace_Overlay branch from dac9ac8 to 9a694fc Compare June 7, 2024 05:12
@vogella vogella force-pushed the MW_FindReplace_Overlay branch from 9a694fc to 21f43d3 Compare June 7, 2024 08:48
vogella and others added 2 commits June 7, 2024 12:06
This PR implements and tests a find/replace dialog which can be
overlayed on top of the editor. The overlay uses the FindReplaceLogic
which is also used by the existing find/replace dialog.
The overlay can be enabled and disabled in the preferences.

eclipse-platform#1090
@vogella vogella force-pushed the MW_FindReplace_Overlay branch from 21f43d3 to 3a2e413 Compare June 7, 2024 10:07
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.

10 participants