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

New option in the context menu for using the value as filter #1182

Merged
merged 2 commits into from Oct 21, 2017

Conversation

Projects
None yet
3 participants
@mgrojo
Copy link
Contributor

mgrojo commented Oct 20, 2017

Added an option in the context menu of the extended table widget for using the currently selected cell value as filter for this column. This allows quick filtering by visible values.

Added an option in the context menu for using the currently selected …
…cell

as filter in this column. This allows quick filtering by selected values.
@MKleusberg
Copy link
Member

MKleusberg left a comment

Thank you for this PR. This is a very good idea and a pretty good and simple implementation 👍 I've found just one minor issue. I hope I can explain it well enough 😉

if (!index.isValid() || !selectionModel()->hasSelection())
return;

m_tableHeader->setFilter(index.column(), "=" + model()->data(index).toString());

This comment has been minimized.

@MKleusberg

MKleusberg Oct 20, 2017

Member

One minor thing here: you should probably use the Qt::EditRole in the data() call here. We use that to get the actual, unmodified cell data instead of what is shown to the user. Without this, 1) a low 'Symbol limit in cell' option can mean that we're filtering for the truncated string, 2) filtering for a BLOB value would literally filter for 'BLOB' which doesn't seem right either, and 3) filtering for NULL values wouldn't work if the 'NULL fields Text' option is set to something other than 'NULL'.

When changing to Qt::EditRole you'll need to check for isNull() though and filter for '=NULL' if that's true. Currently this works sort of automatically but only as long as the null text is set to 'NULL' in the settings.

Changes to pull request #1182 requested by @MKleusberg: get internal …
…cell data and check for the NULL special case.
@mgrojo

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Oct 21, 2017

@MKleusberg I hope I have addressed your requests. Thanks for the feedback.

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Oct 21, 2017

Yes, that's perfect 😄 Thanks again for your PR. It's very much appreciated 😄 If you ever feel like coding some more, you're welcome to do so. If I can somehow help you with that, just email me or open a half-finished PR.

@MKleusberg MKleusberg merged commit 5cef432 into sqlitebrowser:master Oct 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mgrojo

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Oct 21, 2017

Thank you. In fact I'm working on updating the Spanish translation so you can expect at least one more :-)

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Oct 22, 2017

Awesome 😄 Good luck with that 😉

MKleusberg added a commit that referenced this pull request Oct 27, 2017

Fixed crash when editing the display format while the currently brows…
…ed table name is actually a view (#1196)

* Added an option in the context menu for using the currently selected cell
as filter in this column. This allows quick filtering by selected values.

* Changes to pull request #1182 requested by @MKleusberg: get internal cell data and check for the NULL special case.

* Updated Spanish translation.
Reviewed translation following guidelines the KDE Spanish Translation Team: https://es.l10n.kde.org/
Warnings raised by linguist have been solved.

* Fixed crashed when editing the display format and the currently browsed table name is actually a view.

Now it is checked the object type before the cast. This avoids the crash and the field name is obtained for each case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment