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

Replaced case sensitive checkbox with a button using an icon #646

Merged
merged 1 commit into from
Jun 23, 2021

Conversation

Mailaender
Copy link
Contributor

Closes #645.

@eselmeister
Copy link
Contributor

@Mailaender Please have a look at the comment.

@Mailaender Mailaender force-pushed the case-sensitive-button branch from f399612 to f2d35f2 Compare June 21, 2021 13:32
@Mailaender Mailaender requested a review from eselmeister June 21, 2021 13:42
@Mailaender Mailaender force-pushed the case-sensitive-button branch from f2d35f2 to 1c1cc10 Compare June 21, 2021 14:07
@eselmeister
Copy link
Contributor

@Mailaender A final change request:

private Button caseSensitiveButton;
Is not used any longer, as the button directly changes the preference. The member variable can be removed and the creation of the button can be simply called via:

createButtonCaseSensitive(composite);

I would also recommend to set a tooltip, depending on the status of button, e.g.:
button.setToolTipText(PreferenceSupplier.isSearchCaseSensitive() ? "Search Case Sensitive" : "Search Normal");

@Mailaender Mailaender force-pushed the case-sensitive-button branch from 1c1cc10 to e209c6a Compare June 21, 2021 15:37
@Mailaender
Copy link
Contributor Author

Added the tooltip, removed the unused button variable, added boolean caching for performance.

@eselmeister
Copy link
Contributor

@Mailaender Thanks. It looks better, but it contains a possibility to get into an inconsistent status. Have a look at the following case:

A) The search UI is created, Case Sensitive = false, the button is not decorated
B) Then the user modifies somewhere else or in the settings the Case Sensitive preference to true
C) Now run a search with the search UI created at A), the button is still not decorated, but the run search is operated with the variable caseSensitive = true. This could lead to confusing results.

Hence, I would recommend to change the code here:

@Override
public void widgetSelected(SelectionEvent e) {

	caseSensitive = !caseSensitive;
	PreferenceSupplier.setSearchCaseSensitive(caseSensitive);
	button.setToolTipText(caseSensitive ? "Search Case Sensitive" : "Search Case Insensitive");
	button.setImage(ApplicationImageFactory.getInstance().getImage(IApplicationImage.IMAGE_CASE_SENSITIVE, IApplicationImage.SIZE_16x16, caseSensitive));
	runSearch();
}

and here:

if(searchListener != null) {
	String searchText = text.getText().trim();
	searchListener.performSearch(searchText, caseSensitive);
}

Whenever the user changes the search case sensitive flag, the user selection is persisted. But the search UI itself has its own status.

Please let me know if you'd like to discuss this issue.

@Mailaender Mailaender force-pushed the case-sensitive-button branch from e209c6a to af3f7b1 Compare June 22, 2021 09:14
@Mailaender
Copy link
Contributor Author

Added your changes, but I am still fighting a nightmarish bug here that freezes the whole application. I narrowed it down, but I don't understand why it is a problem. Left a source code comment.

@eselmeister
Copy link
Contributor

OK, let me review it.

@eselmeister
Copy link
Contributor

That is a tricky one :-).
The plain image needs to be set first. Then, the decorator could come into play.

Have a look at the code to solve the issue:

/*******************************************************************************
 * Copyright (c) 2017, 2021 Lablicate GmbH.
 * 
 * All rights reserved. This program and the accompanying materials
 * are made available under the terms of the Eclipse Public License v1.0
 * which accompanies this distribution, and is available at
 * http://www.eclipse.org/legal/epl-v10.html
 * 
 * Contributors:
 * Dr. Philip Wenig - initial API and implementation
 *******************************************************************************/
package org.eclipse.chemclipse.swt.ui.components;

import org.eclipse.chemclipse.model.preferences.PreferenceSupplier;
import org.eclipse.chemclipse.rcp.ui.icons.core.ApplicationImageFactory;
import org.eclipse.chemclipse.rcp.ui.icons.core.IApplicationImage;
import org.eclipse.swt.SWT;
import org.eclipse.swt.events.KeyAdapter;
import org.eclipse.swt.events.KeyEvent;
import org.eclipse.swt.events.SelectionAdapter;
import org.eclipse.swt.events.SelectionEvent;
import org.eclipse.swt.layout.FillLayout;
import org.eclipse.swt.layout.GridData;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Button;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.swt.widgets.Text;

public class SearchSupportUI extends Composite {

	private Text text;
	private Button buttonCaseSensitive;
	//
	private ISearchListener searchListener;
	private boolean caseSensitive = PreferenceSupplier.isSearchCaseSensitive();

	public SearchSupportUI(Composite parent, int style) {

		super(parent, style);
		createControl();
	}

	@Override
	public void setVisible(boolean visible) {

		super.setVisible(visible);
		updateButtonCaseSensitive();
	}

	public void reset() {

		setSearchText("");
	}

	public void setSearchListener(ISearchListener searchListener) {

		this.searchListener = searchListener;
	}

	public void setSearchText(String searchText) {

		text.setText(searchText);
		runSearch();
	}

	/**
	 * Use this e.g. for content proposal listeners.
	 * The search text shall be set via the setSearchText method.
	 * 
	 * @return Text
	 */
	public Text getText() {

		return text;
	}

	public String getSearchText() {

		return text.getText().trim();
	}

	public boolean isSearchCaseSensitive() {

		return caseSensitive;
	}

	private void createControl() {

		setLayout(new FillLayout());
		//
		Composite composite = new Composite(this, SWT.NONE);
		GridLayout gridLayout = new GridLayout(3, false);
		gridLayout.marginLeft = 0;
		gridLayout.marginRight = 0;
		composite.setLayout(gridLayout);
		//
		createButtonSearch(composite);
		text = createTextSearch(composite);
		buttonCaseSensitive = createButtonCaseSensitive(composite);
	}

	private Button createButtonSearch(Composite parent) {

		Button button = new Button(parent, SWT.PUSH);
		button.setText("");
		button.setToolTipText("Run the search");
		button.setImage(ApplicationImageFactory.getInstance().getImage(IApplicationImage.IMAGE_SEARCH, IApplicationImage.SIZE_16x16));
		button.addSelectionListener(new SelectionAdapter() {

			@Override
			public void widgetSelected(SelectionEvent e) {

				runSearch();
			}
		});
		//
		return button;
	}

	private Text createTextSearch(Composite parent) {

		Text text = new Text(parent, SWT.BORDER | SWT.SEARCH | SWT.ICON_CANCEL | SWT.ICON_SEARCH);
		text.setText("");
		text.setToolTipText("Type in the search items.");
		text.setLayoutData(new GridData(GridData.FILL_HORIZONTAL));
		/*
		 * Listen to search key event.
		 */
		text.addKeyListener(new KeyAdapter() {

			@Override
			public void keyReleased(KeyEvent e) {

				if(e.keyCode == SWT.LF || e.keyCode == SWT.CR || e.keyCode == SWT.KEYPAD_CR) {
					runSearch();
				} else if(text.getText().trim().equals("")) {
					/*
					 * Reset when empty.
					 */
					runSearch();
				}
			}
		});
		/*
		 * Click on the icons.
		 */
		text.addSelectionListener(new SelectionAdapter() {

			@Override
			public void widgetDefaultSelected(SelectionEvent e) {

				if(e.detail == SWT.ICON_CANCEL) {
					text.setText("");
					runSearch();
				} else if(e.detail == SWT.ICON_SEARCH) {
					runSearch();
				}
			}
		});
		//
		return text;
	}

	private Button createButtonCaseSensitive(Composite parent) {

		Button button = new Button(parent, SWT.PUSH);
		button.setText("");
		button.setToolTipText("");
		button.setImage(ApplicationImageFactory.getInstance().getImage(IApplicationImage.IMAGE_CASE_SENSITIVE, IApplicationImage.SIZE_16x16));
		button.addSelectionListener(new SelectionAdapter() {

			@Override
			public void widgetSelected(SelectionEvent e) {

				caseSensitive = !caseSensitive;
				PreferenceSupplier.setSearchCaseSensitive(caseSensitive);
				updateButtonCaseSensitive();
				runSearch();
			}
		});
		//
		return button;
	}

	private void updateButtonCaseSensitive() {

		buttonCaseSensitive.setToolTipText(caseSensitive ? "Search Case Sensitive" : "Search Case Insensitive");
		buttonCaseSensitive.setImage(ApplicationImageFactory.getInstance().getImage(IApplicationImage.IMAGE_CASE_SENSITIVE, IApplicationImage.SIZE_16x16, caseSensitive));
	}

	private void runSearch() {

		if(searchListener != null) {
			String searchText = text.getText().trim();
			searchListener.performSearch(searchText, caseSensitive);
		}
	}
}

@Mailaender Mailaender force-pushed the case-sensitive-button branch from af3f7b1 to 343ac8c Compare June 22, 2021 20:30
@Mailaender
Copy link
Contributor Author

Interesting.

button.setImage(ApplicationImageFactory.getInstance().getImage(IApplicationImage.IMAGE_BAR_CHART, IApplicationImage.SIZE_16x16, colorCompensation));
is somehow not effected by this bug.

@eselmeister
Copy link
Contributor

The WellChart example is interesting. It could be, that barchart has been used before, hence it's in the cache already. Let's do a further inspection on this.

The SearchSupportUI code looks fine now.

@eselmeister eselmeister merged commit 9ecf334 into eclipse:develop Jun 23, 2021
@Mailaender Mailaender deleted the case-sensitive-button branch June 23, 2021 08:52
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.

Unlabeled checkbox for case sensitive search is bad UX
2 participants