Enhance the SQL-dataset-editor with a search option (#1700) #1701

speckyspooky · 2024-05-21T19:01:59Z

No description provided.

...a.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java

merks · 2024-05-21T19:21:26Z

...a.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java

+
+	private void setupLabelFindQueryTextGroup(int findMode) {
+		Device device = Display.getCurrent();
+		Color fontColor = new Color(device, 0, 0, 0);


Assuming you’re not in dark mode?

When the color of the label is changed to "red" for the hint message it will be set back to default-color black.
Therefore I define the default color. We need the color-reset because if not the label would be always red when a red-message was displayed.

What color is the text in dark mode? I’m on a phone so it limits the links to send. Setting to black assumes the text is black. Not a good assumption. Also Color needs to be disposed. Better to look how other parts of the framework get colors and set them.

You are right, black text with black background is really hard.
I store the original color on a privat member and the label color will be disposed after use directly.

Done and optimized.

merks · 2024-05-21T20:38:31Z

It would be really good to learn how to amend a commit and force push it so that a PR is a single commit in the history with no semi-bogus intermediate commits.

merks

There is a fair bit of room for improvement, but the color handling is especially poor.

merks · 2024-05-22T05:40:25Z

...a.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java

+			}
+
+			private boolean isFindQueryTextBackward(KeyEvent e) {
+				// CTRL + SHIFT + f


I don't think the comment matches the code.

I will fix it

merks · 2024-05-22T05:41:29Z

...a.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java

@@ -149,6 +156,11 @@ public class SQLDataSetEditorPage extends DataSetWizardPage {
 	private static final int DB_OBJECT_TREE_HEIGHT_MIN = 150;
 	private static final int DB_OBJECT_TREE_WIDTH_MIN = 200;

+	private static final String FIND_DIRECTION_SYMBOLE_FORWARD = "\u25BC";


The German spelling for symbol is has kicked it.

I will fix it

merks · 2024-05-22T05:42:00Z

...a.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java

+	private static final String FIND_DIRECTION_SYMBOLE_FORWARD = "\u25BC";
+	private static final String FIND_DIRECTION_SYMBOLE_BACKWARD = "\u25B2";
+
+	private Color findGroupLabelOriginalColor = null;


I don't think this is needed. I expect you can get the color from some other textual thing in the dialog.

In my point of view it is better to use the original element instead of another text element of the dialog.

merks · 2024-05-22T05:43:52Z

...a.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java

@@ -998,6 +1031,21 @@ private boolean isRedoKeyPress(KeyEvent e) {
 				return ((e.stateMask & SWT.CONTROL) > 0) && ((e.keyCode == 'y') || (e.keyCode == 'Y'));
 			}

+			private boolean isFindQueryText(KeyEvent e) {
+				// CTRL + f
+				return ((e.stateMask & SWT.CONTROL) > 0) && ((e.keyCode == 'f') || (e.keyCode == 'F'));


In other places in the Platform I see tests like this:

if (e.stateMask == SWT.CTRL && e.keyCode == 'a') {

That makes me wonder what cases are all being covered by your guard other than ctrl-f?

This verbose and to me confusing pattern is repeated often...

Here I used the original pattern which was used in this viewer part.
Yes, I know, your example is more clean, so I will change it to your version.

All keyboard keys changed to your way - also the old versions (!)

merks · 2024-05-22T05:49:04Z

...a.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java

+
+		// forward search end
+		if (findMode == 1) {
+			fontColor = new Color(Display.getCurrent(), 255, 0, 0);


There are many better ways to get colors and better ways to get the display. E.g.,

findQueryTextGroup.getDisplay().getSystemColor(SWT.COLOR_RED);

But look at how the Platform handles such a case of a status error in the find and replace dialog:

https://github.com/eclipse-platform/eclipse.platform.ui/blob/master/bundles/org.eclipse.ui.workbench.texteditor/src/org/eclipse/ui/texteditor/FindReplaceDialog.java#L1310-L1324

Nowhere is the color hard coded but rather is controlled by the user's preferences that are sensitive to the color mode.

I will change it.

merks · 2024-05-22T05:53:10Z

...a.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java

+		if (findQueryText != null) {
+			boolean found = viewer.findQueryText(findQueryText.getText(), true,
+					findQueryTextCaseSensitive.getSelection(), findQueryTextWholeWord.getSelection());
+			if (found) {


More concise would be setupLabelFindQueryTextGroup(found ? 0 : 1).

I will change it.

merks · 2024-05-22T05:54:35Z

...a.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java

@@ -1075,4 +1123,149 @@ private void insertTreeItemText() {
 			insertText(text);
 		}
 	}
+
+	private void setupFindQueryTextBox(Group group) {
+		this.findQueryText = new Text(group, SWT.BORDER);


There is a lot of this use of this where it just isn't necessary. I expect that to be used only when there is a local variable with the same name.

The use of "this" is my personal style, I will remove it.

merks · 2024-05-22T05:55:21Z

...a.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java

+	private void setupFindQueryTextOptions(Group group) {
+
+		GridData csLayoutData = new GridData(GridData.HORIZONTAL_ALIGN_BEGINNING);
+		this.findQueryTextCaseSensitive = new Button(group, SWT.CHECK);


Too many pointless this.

I will remove it.

...a.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java

merks · 2024-05-22T06:00:57Z

...a.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java

+
+		findQueryTextGroup.setForeground(fontColor);
+		findQueryTextGroup.setText(groupLabel);
+		fontColor.dispose();


Wow, does this even work? Setting the color doesn't copy so you can't dispose something that's in use. I have a feeling that Color doesn't actually do anything when disposed, on some or maybe all platforms, but in any case this is a super bad pattern to follow.

What is your suggestion for the Color-dispose?

The dispose will work!
Because that this is the reason why I copy the color-object to a new object value.
If I use the same reference without copy then the dispose destroy/dispose really the color object and it isn't available any more.

If disposed did something then the widget using the color would be broken.

Please look at how the platform is doing the same thing as I've referenced elsewhere.

I will change it.

merks · 2024-05-22T06:12:51Z

Here is how to amend. You must push the amend toolbar button:

Once pushed, it will bring back the commit message of the current commit:

(It's possible to unpush the button too.)

You can add any changes to the commit being amended via what is staged.

Then Commit it; Commit and Push will fail to do the push because the changes are not fast-forward.

Bring up up the Push... dialog on the branch via the context menu:

In the dialog, it's generally best to use Rebase, but most importantly, check mark the Force option. Then you can complete the operation. The PR's commit will be updated and a new build will kick off.

merks · 2024-05-22T07:03:41Z

Please, please, do the colors similar to like this:

	private void evaluateFindReplaceStatus(boolean allowBeep) {
		IFindReplaceStatus status = findReplaceLogic.getStatus();

		String dialogMessage = status.accept(new FindReplaceLogicMessageGenerator());
		fStatusLabel.setText(dialogMessage);
		if (status.isInputValid()) {
			fStatusLabel.setForeground(fReplaceLabel.getForeground());
		} else {
			fStatusLabel.setForeground(JFaceColors.getErrorText(fStatusLabel.getDisplay()));
		}

		if (!status.wasSuccessful()) {
			tryToBeep(allowBeep);
		}
	}

No hard coded colors, no copying, no disposing. The colors of fonts within a dialog is consistently the same, so assuming that you need to keep the original color is overkill. But in any case, don't create colors and don't dispose them.

speckyspooky · 2024-05-22T07:29:49Z

Please, please, do the colors similar to like this:
	private void evaluateFindReplaceStatus(boolean allowBeep) {
...
	}
No hard coded colors, no copying, no disposing. The colors of fonts within a dialog is consistently the same, so assuming that you need to keep the original color is overkill. But in any case, don't create colors and don't dispose them.

I will use this pattern to set the color setting and will remove the dispose.

speckyspooky · 2024-05-22T19:00:59Z

@merks
The color handling is optimized according to your suggestion.
I have reused the last commit and there isn't an additional commit listed.

merks · 2024-05-22T19:11:33Z

It looks very promising! I’ll look tomorrow when I’m back at the computer.

merks · 2024-05-23T10:08:09Z

FYI, SimRel disaster kicked in this morning:

https://www.eclipse.org/lists/cross-project-issues-dev/msg19901.html

I've temporarily disable BIRT's builds because of this problem; I don't want any badly signing content in the update sites. I hope the certificate will be resolved in the coming hours.

merks

Very good. Thanks for the improvements.

speckyspooky · 2024-05-23T10:43:13Z

Thanks for your time and detailed support!

Enhance the SQL-dataset-editor with a search option (eclipse-birt#1700)

b8dc350

speckyspooky added the Feature label May 21, 2024

speckyspooky added this to the 4.16 milestone May 21, 2024

speckyspooky requested a review from merks May 21, 2024 19:01

speckyspooky self-assigned this May 21, 2024

speckyspooky requested a review from wimjongman May 21, 2024 19:02

merks requested changes May 21, 2024

View reviewed changes

Set symbol-sign unicode-encoded and simplify the button events

dc8fb67

speckyspooky requested a review from merks May 22, 2024 04:18

merks requested changes May 22, 2024

View reviewed changes

Optimize color handling and source code

1a075fb

speckyspooky force-pushed the enhance_sql_dataset_editor_with_search branch from 0c54dec to 1a075fb Compare May 22, 2024 18:54

merks approved these changes May 23, 2024

View reviewed changes

speckyspooky merged commit 833a176 into eclipse-birt:master May 23, 2024
3 of 12 checks passed

merks mentioned this pull request May 24, 2024

do not autosize columns in case of StreamingXlsx eclipse-birt#1703 #1704

Merged

merks mentioned this pull request Jun 4, 2024

Move RadioGroup from incubation to release eclipse/nebula#598

Merged

Enhance the SQL-dataset-editor with a search option (#1700) #1701

Enhance the SQL-dataset-editor with a search option (#1700) #1701

Conversation

speckyspooky commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merks commented May 21, 2024

merks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merks commented May 22, 2024

merks commented May 22, 2024

speckyspooky commented May 22, 2024

speckyspooky commented May 22, 2024

merks commented May 22, 2024

merks commented May 23, 2024

merks left a comment

Choose a reason for hiding this comment

speckyspooky commented May 23, 2024