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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
May 23, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8dc350
Enhance the SQL-dataset-editor with a search option (#1700)
speckyspooky May 21, 2024
dc8fb67
Set symbol-sign unicode-encoded and simplify the button events
speckyspooky May 21, 2024
1a075fb
Optimize color handling and source code
speckyspooky May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
199 changes: 194 additions & 5 deletions ...da.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLDataSetEditorPage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2004, 2013 Actuate Corporation.
* Copyright (c) 2004, 2013 Actuate Corporation, 2024 and others
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License 2.0 which is available at
Expand All @@ -10,6 +10,7 @@
*
* Contributors:
* Actuate Corporation - initial API and implementation
* Thomas Gutmann - add query text search
*******************************************************************************/

package org.eclipse.birt.report.data.oda.jdbc.ui.editors;
Expand Down Expand Up @@ -81,12 +82,15 @@
import org.eclipse.swt.events.MouseTrackAdapter;
import org.eclipse.swt.events.SelectionAdapter;
import org.eclipse.swt.events.SelectionEvent;
import org.eclipse.swt.graphics.Color;
import org.eclipse.swt.graphics.Device;
import org.eclipse.swt.layout.GridData;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Button;
import org.eclipse.swt.widgets.Combo;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.swt.widgets.Control;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Event;
import org.eclipse.swt.widgets.Group;
import org.eclipse.swt.widgets.Label;
Expand All @@ -109,8 +113,9 @@ public class SQLDataSetEditorPage extends DataSetWizardPage {

// composite in editor page
private Document doc = null;
private SourceViewer viewer = null;
speckyspooky marked this conversation as resolved.
Show resolved Hide resolved
private SQLSourceViewer viewer = null;
private Text searchTxt = null;
private Text findQueryText = null;
private ComboViewer filterComboViewer = null;
private Combo schemaCombo = null;
private Menu treeMenu = null;
Expand All @@ -123,9 +128,12 @@ public class SQLDataSetEditorPage extends DataSetWizardPage {
private Button showSystemTableCheckBox = null;
private Button showAliasCheckBox = null;
private Button includeSchemaCheckBox = null;
private Button findQueryTextWholeWord = null;
private Button findQueryTextCaseSensitive = null;
private Exception prepareException = null;
private Group sqlOptionGroup = null;
private Group selectTableGroup = null;
private Group findQueryTextGroup = null;

private static String DEFAULT_MESSAGE = JdbcPlugin.getResourceString("dataset.new.query");//$NON-NLS-1$

Expand Down Expand Up @@ -870,7 +878,6 @@ private String getTextToInsert() {

/**
* Adds drop support to viewer.Must set viewer before execution.
*
*/
private void addDropSupportToViewer() {
final StyledText text = viewer.getTextWidget();
Expand Down Expand Up @@ -944,11 +951,27 @@ private Control createTextualQueryComposite(Composite parent) {
CompositeRuler ruler = new CompositeRuler();
LineNumberRulerColumn lineNumbers = new LineNumberRulerColumn();
ruler.addDecorator(0, lineNumbers);
viewer = new SourceViewer(composite, ruler, SWT.H_SCROLL | SWT.V_SCROLL);
viewer = new SQLSourceViewer(composite, ruler, SWT.H_SCROLL | SWT.V_SCROLL);
SourceViewerConfiguration svc = new SQLSourceViewerConfiguration(dataSetDesign.getDataSourceDesign(),
timeOutLimit * 1000, enableCodeAssist);
viewer.configure(svc);

// Find query text at source viewer
findQueryTextGroup = new Group(composite, SWT.FILL);
findQueryTextGroup.setText("Find"); //$NON-NLS-1$
GridLayout findQueryTextGroupLayout = new GridLayout();
findQueryTextGroupLayout.verticalSpacing = 10;
findQueryTextGroupLayout.numColumns = 4;
findQueryTextGroup.setLayout(findQueryTextGroupLayout);
GridData fOptionGroupData = new GridData(GridData.FILL_HORIZONTAL);
findQueryTextGroup.setLayoutData(fOptionGroupData);

setupFindQueryTextBox(findQueryTextGroup);

setupFindQueryTextButtons(findQueryTextGroup);

setupFindQueryTextOptions(findQueryTextGroup);

doc = new Document(getQueryText());
FastPartitioner partitioner = new FastPartitioner(new SQLPartitionScanner(), new String[] {
SQLPartitionScanner.QUOTE_STRING, SQLPartitionScanner.COMMENT, IDocument.DEFAULT_CONTENT_TYPE });
Expand Down Expand Up @@ -976,7 +999,7 @@ public void lineGetSegments(BidiSegmentEvent event) {
// Add drop support to the viewer
addDropSupportToViewer();

// add support of additional accelerated key
// Add support of additional accelerated key
viewer.getTextWidget().addKeyListener(new KeyListener() {

@Override
Expand All @@ -985,6 +1008,12 @@ public void keyPressed(KeyEvent e) {
viewer.doOperation(ITextOperationTarget.UNDO);
} else if (isRedoKeyPress(e)) {
viewer.doOperation(ITextOperationTarget.REDO);
} else if (isFindQueryText(e)) {
findQueryText.setFocus();
} else if (isFindQueryTextForward(e)) {
findQueryTextForward();
} else if (isFindQueryTextBackward(e)) {
findQueryTextBackward();
}
}

Expand All @@ -998,6 +1027,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'));
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

private boolean isFindQueryTextForward(KeyEvent e) {
// CTRL + o
return ((e.stateMask & SWT.CONTROL) > 0) && ((e.keyCode == 'o') || (e.keyCode == 'O'));
}

private boolean isFindQueryTextBackward(KeyEvent e) {
// CTRL + b
return ((e.stateMask & SWT.CONTROL) > 0) && ((e.keyCode == 'b') || (e.keyCode == 'B'));
}

@Override
public void keyReleased(KeyEvent e) {
}
Expand Down Expand Up @@ -1075,4 +1119,149 @@ private void insertTreeItemText() {
insertText(text);
}
}

private void setupFindQueryTextBox(Group group) {
this.findQueryText = new Text(group, SWT.BORDER);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

this.findQueryText.setToolTipText(JdbcPlugin.getResourceString("tablepage.querytext.find.text.tooltip"));
GridData findQueryTextData = new GridData(GridData.FILL_HORIZONTAL);
findQueryTextData.horizontalSpan = 2;
this.findQueryText.setLayoutData(findQueryTextData);
// add support of additional accelerated key
findQueryText.addKeyListener(new KeyListener() {

@Override
public void keyPressed(KeyEvent e) {
if (isFindQueryTextForward(e)) {
findQueryTextForward();
} else if (isFindQueryTextBackward(e)) {
findQueryTextBackward();
}
}

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

private boolean isFindQueryTextBackward(KeyEvent e) {
// CTRL + SHIFT + f
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the comment matches the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

return ((e.stateMask & SWT.CONTROL) > 0) && ((e.keyCode == 'b') || (e.keyCode == 'B'));
}

@Override
public void keyReleased(KeyEvent e) {
}

});

}

private void setupFindQueryTextOptions(Group group) {

GridData csLayoutData = new GridData(GridData.HORIZONTAL_ALIGN_BEGINNING);
this.findQueryTextCaseSensitive = new Button(group, SWT.CHECK);
Copy link
Contributor

Choose a reason for hiding this comment

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

Too many pointless this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

this.findQueryTextCaseSensitive
.setText(JdbcPlugin.getResourceString("tablepage.querytext.find.option.case.sensitive")); //$NON-NLS-1$
this.findQueryTextCaseSensitive
.setToolTipText(JdbcPlugin.getResourceString("tablepage.querytext.find.option.case.sensitive.tooltip")); //$NON-NLS-1$
this.findQueryTextCaseSensitive.setSelection(false);
this.findQueryTextCaseSensitive.setLayoutData(csLayoutData);
this.findQueryTextCaseSensitive.setEnabled(true);

GridData wwLayoutData = new GridData(GridData.HORIZONTAL_ALIGN_BEGINNING);
this.findQueryTextWholeWord = new Button(group, SWT.CHECK);
this.findQueryTextWholeWord.setText(JdbcPlugin.getResourceString("tablepage.querytext.find.option.whole.word")); //$NON-NLS-1$
this.findQueryTextWholeWord
.setToolTipText(JdbcPlugin.getResourceString("tablepage.querytext.find.option.whole.word.tooltip")); //$NON-NLS-1$
this.findQueryTextWholeWord.setSelection(false);
this.findQueryTextWholeWord.setLayoutData(wwLayoutData);
this.findQueryTextWholeWord.setEnabled(true);
}

private void setupFindQueryTextButtons(Group group) {

GridData fwButtonFindTextLayoutData = new GridData(GridData.HORIZONTAL_ALIGN_BEGINNING);
Button fwButtonFindText = new Button(group, SWT.BUTTON1);
fwButtonFindText.setText("▼ " + JdbcPlugin.getResourceString("tablepage.querytext.find.button.forward")); //$NON-NLS-1$
speckyspooky marked this conversation as resolved.
Show resolved Hide resolved
fwButtonFindText.setToolTipText(JdbcPlugin.getResourceString("tablepage.querytext.find.button.forward.tooltip")); //$NON-NLS-1$
fwButtonFindText.setLayoutData(fwButtonFindTextLayoutData);
fwButtonFindText.setEnabled(true);
// Add listener to the find query button "forward"
fwButtonFindText.addSelectionListener(new SelectionAdapter() {

@Override
public void widgetSelected(SelectionEvent event) {
PlatformUI.getWorkbench().getDisplay().asyncExec(new Runnable() {

@Override
public void run() {
findQueryTextForward();
}
});
}
});

GridData bwButtonFindTextLayoutData = new GridData(GridData.HORIZONTAL_ALIGN_BEGINNING);
Button bwButtonFindText = new Button(group, SWT.BUTTON1);
bwButtonFindText.setText("▲ " + JdbcPlugin.getResourceString("tablepage.querytext.find.button.backward")); //$NON-NLS-1$
speckyspooky marked this conversation as resolved.
Show resolved Hide resolved
bwButtonFindText.setToolTipText(JdbcPlugin.getResourceString("tablepage.querytext.find.button.backward.tooltip")); //$NON-NLS-1$
bwButtonFindText.setLayoutData(bwButtonFindTextLayoutData);
bwButtonFindText.setEnabled(true);
// Add listener to the find query button "forward"
bwButtonFindText.addSelectionListener(new SelectionAdapter() {

@Override
public void widgetSelected(SelectionEvent event) {
PlatformUI.getWorkbench().getDisplay().asyncExec(new Runnable() {
speckyspooky marked this conversation as resolved.
Show resolved Hide resolved

@Override
public void run() {
findQueryTextBackward();
}
});
}
});
}

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

Choose a reason for hiding this comment

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

Assuming you’re not in dark mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and optimized.

String groupLabel = JdbcPlugin.getResourceString("tablepage.querytext.find.label");

// forward search end
if (findMode == 1) {
fontColor = new Color(device, 255, 0, 0);
groupLabel += ", " + JdbcPlugin.getResourceString("tablepage.querytext.find.forward.unlocated.label");
// backward search end
} else if (findMode == 2) {
fontColor = new Color(device, 255, 0, 0);
groupLabel += ", " + JdbcPlugin.getResourceString("tablepage.querytext.find.backward.unlocated.label");
}
findQueryTextGroup.setForeground(fontColor);
findQueryTextGroup.setText(groupLabel);
}

private void findQueryTextForward() {
if (findQueryText != null) {
boolean found = viewer.findQueryText(findQueryText.getText(), true,
findQueryTextCaseSensitive.getSelection(), findQueryTextWholeWord.getSelection());
if (found) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

setupLabelFindQueryTextGroup(0);
} else {
setupLabelFindQueryTextGroup(1);
}
}
}

private void findQueryTextBackward() {
if (findQueryText != null) {
boolean found = viewer.findQueryText(findQueryText.getText(), false,
findQueryTextCaseSensitive.getSelection(), findQueryTextWholeWord.getSelection());
if (found) {
setupLabelFindQueryTextGroup(0);
} else {
setupLabelFindQueryTextGroup(2);
}
}
}
}
101 changes: 101 additions & 0 deletions ...ata.oda.jdbc.ui/src/org/eclipse/birt/report/data/oda/jdbc/ui/editors/SQLSourceViewer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
/*******************************************************************************
* Copyright (c) 2023 Thomas Gutmann.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License 2.0 which is available at
* https://www.eclipse.org/legal/epl-2.0/.
*
* SPDX-License-Identifier: EPL-2.0
*
*
* Contributors:
* Thomas Gutmann - initial API and implementation
*******************************************************************************/
package org.eclipse.birt.report.data.oda.jdbc.ui.editors;

import org.eclipse.jface.text.IFindReplaceTarget;
import org.eclipse.jface.text.ITextSelection;
import org.eclipse.jface.text.source.IVerticalRuler;
import org.eclipse.jface.text.source.SourceViewer;
import org.eclipse.swt.widgets.Composite;

/**
* Extended standard source viewer for query text search
*
* @since 4.16
*
*/
public class SQLSourceViewer extends SourceViewer {

/**
* Constructor
*
* @param parent parent element
* @param ruler vertical ruler
* @param styles editor style
*/
public SQLSourceViewer(Composite parent, IVerticalRuler ruler, int styles) {
super(parent, ruler, styles);
}

/**
* Get the current cursor position from source viewer
*
* @return the current cursor position
*/
public int getCurrentCaretPosition() {
return this.getTextWidget().getCaretOffset();
}

/**
* Find query text and mark
*
* @param findQueryText query text to be searched
* @param forwardSearch search is forward search (or backward search)
* @param queryTextCaseSensitive search is case sensitive
* @param queryTextWholeWord the text is searched as a whole word
* @return query text found
*/
public boolean findQueryText(String findQueryText, boolean forwardSearch, boolean queryTextCaseSensitive,
boolean queryTextWholeWord) {

boolean textFound = false;
int queryTextLength = this.getTextWidget().getText().length();
int startPosition = getCurrentCaretPosition();
int searchPositionFound = -1;

ITextSelection selection = (ITextSelection) this.getSelectionProvider().getSelection();
if (!forwardSearch && selection.getLength() > 0)
startPosition -= selection.getLength();

IFindReplaceTarget frt = this.getFindReplaceTarget();
if (frt != null && frt.canPerformFind() && findQueryText != null) {

int textLength = findQueryText.length();
if (textLength > 0) {

searchPositionFound = frt.findAndSelect(startPosition, findQueryText, forwardSearch,
queryTextCaseSensitive,
queryTextWholeWord);

if (searchPositionFound >= 0) {
textFound = true;

// set the caret
if ((searchPositionFound + textLength) <= queryTextLength) {
this.getTextWidget().setCaretOffset(searchPositionFound + textLength);
}

// set the selection
int[] selectionRange = { searchPositionFound, textLength };
this.validateSelectionRange(selectionRange);
if (selectionRange[0] > 0) {
this.setSelectedRange(searchPositionFound, textLength);
}
}
}
}
return textFound;
}

}
Loading
Loading