Обсуждение: Fwd: Filter by Selection on Grid

Hi,

On Feb 1, 2008 4:38 AM, Robins Tharakan <tharakan@gmail.com> wrote:
> The good part is that now the patch does the following:
> 1. Provide options for 'Filter by Selection' / 'Filter Excluding Selection'
> / 'Remove Filter'
>  2. Takes care if cells have NULL value
> 3. Takes multiple Include / Exclude selections and refreshes automatically

Looks good :-)

Some minor comments:

- the _() macro is used for strings that need translation, so please
use wxT() for those that don't, such as parts of SQL queries.

- I think we should have the menu options on a new Tools menu as well
(we normally duplicate context menu options onto the main menu for
accessibility reasons). We should probably move the Sort/Filter option
onto there as well.

> Limitations that I plan to remove soon:
> 1. The cell chosen is the 'Selected cell' and that may not be the one
> right-clicked. I'd change that soon.

Yeah - I've actually been thinking that for various reasons we might
want to have the active cell follow the mouse clicks and row/column
selections anyway.

>  2. I am unable to evaluate strings if they are Empty, so
> including/excluding empty-cells (not NULLs) doesn't do anything !
> Frustrating ! May be I just need a break, a bit of reading and I'll get that
> done soon as well!

Don't forget that empty strings are displayed as '' (two single
quotes), and a string that actually is two single quotes is \'\'.
You'll need to allow for those when building your filters.

> In case this patch is considered by and large fine, and in case you are ok
> with more additions, I'll try adding more features in the right click menu
> (Filter by Custom String / Sort options etc..) similar to what we have in
> the Access UI.

I look forward to it :-)

Thanks, Dave

Hi Robins,

Robins Tharakan wrote:
> Thanks for the feedback. A positive note certainly helps :)
>

So one more positive note. This is really great. It helps working
quicker with pgAdmin. So, thanks a lot :)

> I've tried to work on the things mentioned and although I can think of a
> few more additions, I think I'd work on them separately once this gets
> through.
>

+1

> A few updates:
> 1. Replaced all SQL related _() with wxT()
> 2. Created a separate Tools Menu. Moved 'Sort / Filter ...' there and
> added all the new 'right-clickable options' there as well.
> 3. Right-clicking a cell makes that cell the selected cell on the grid.
> 4. Empty strings are now taken care of as well.
> 5. As earlier, all options(including the sort options) refresh the grid
> immediately.
>
> Hope I didn't miss anything!
>

The last item on the contextual menu is a separator. I wonder why ?

Regards.


--
Guillaume.
  http://www.postgresqlfr.org
  http://dalibo.com

Hi

On Feb 5, 2008 12:54 PM, Robins Tharakan <robins@pobox.com> wrote:

> > The last item on the contextual menu is a separator. I wonder why ?
> >
> Thats more of a mistake of sorts... Actually I am half way into adding the
> Copy/Paste on the right-click.
> Just that I thought I should finalize on a few UI decisions before I send
> that patch in.

OK, slow down :-). I see the latest version of the patch added sorting
functionality - which is great, but please try not to change the
functionality once we're into the review/refinement process. If you
want to add more functionality, post a new patch, otherwise the
process just becomes a lot more complicated.

Sorting is there now though, so we'll deal with both together.

> But in all this, somehow the separator got pushed in before time :-)

I see two remaining problems with the patch - that being one of them,
and the other being that there is still code in there attempting to
enable/disable the sorting/filter options on the View menu - that
needs to be changed throughout to the Tools menu of course (eg.
viewMenu->Enable(MNU_OPTIONS, false); ), plus the other new options
should be enabled/disabled in the same places. That's done to prevent
these functions being selected during data refresh or other
operations.

One hint - test in a debug enabled build as you should then see
asserts for things like enabling non-existant menu options.

Once thats sorted, I think we'll be ready to apply :-)

Thansk, Dave

On Feb 5, 2008 5:57 PM, Robins Tharakan <robins@pobox.com> wrote:

> Sure, I'll try and keep separate patches in separate emails.
> Attached is reworked patch which takes care of the issues that you
> mentioned.
>
> Updates in this patch:
> 1. Removed the extra separator that came up on right-click.
>  2. Enabled / Disabled the menu options while a query is running
> 3. The code also disables the right-click while a thread->IsRunning() is
> true. (That should effectively disable the right-click when not needed right
> ?)

Looks good - applied with minor changes:

- Check that thread is valid before checking if it's running.
- Properly quote/escape column names and text values.
- Adjust the brace style - specifically, remove braces around single
lines of code, and split the onto individual lines, eg. instead of:

if (x = y) {
  // do stuff
  // in here
} else {
  // do other stuff
}

we prefer:

if (x = y)
{
  // do stuff
  // in here
}
else
  // do other stuff

One other thing I noticed that should be fixed in a new patch - if you
sort ascending on a column, and then sort descending, it will add
both. It should remove the ascending and replace it with the
descending sort.

But... an excellent first patch! I hope you'll stick around and come
up with a few more similarly useful features :-)

Thanks, Dave

On Feb 6, 2008 11:02 AM, Robins Tharakan <tharakan@gmail.com> wrote:
> > ... snip ...
> Sure ! Points that I'll take into account from now on.
>
>
> > One other thing I noticed that should be fixed in a new patch - if you
>  > sort ascending on a column, and then sort descending, it will add
> > both. It should remove the ascending and replace it with the
>  > descending sort.
>
> True... and one more thing that I was working on (when I responded to
> Guillaume the other day) was that if a person does an 'exclude by selection'
> on a field with a value = 10, we simply put a [WHERE] value <> 10 filter...
> however I think this is incomplete.
>
> instead of
> WHERE value <> 10
>
> we should rather put
>
> WHERE (value <> 10 OR value IS NULL)
> Or
>  WHERE (value IS DISTINCT FROM 10)
>
> This is because as per the UI, the user does not want value 10, but he still
> would want a NULL value in the records.

Yes, thats a good point. I look forward to seeing the patch ;-)

cheers, Dave.

Hi.

On Feb 6, 2008 5:42 PM, Robins Tharakan <tharakan@gmail.com> wrote:
> Hi,
>
> The attached patch does this:
> 1. Sanitize the if-else bracket style for the earlier patch.
> 2. Use 'IS DISTINCT FROM' instead of <> in an exclusion condition.
>
> Two issues remain that I can't resolve:
> 1. Rightly as you point out thread->IsRunning() needs to be called only if
> it exists. On rigorous testing the application bombs if this thread is
> non-existent (backtrace attached). Any good way to check whether a thread is
> running ? Any flag etc ?

I think you misunderstood my previous message. I committed your patch
to SVN, and cleaned up the formatting, added the test to ensure thread
is valid, and added the proper quoting.

So at this point you should svn update and start working from svn
trunk again to add the IS DISTINCT FROM and duplicate sort prevention.

> 2. The case of selection and deselection of the same field name can
> (ideally) cause a bit of a complication.
> Consider this:
> a. M=10 AND M <> 10
> b. M = 10 AND (N = 10 OR M <> 10)
> If we are to filter out the old test condition replacing it with a new test
> condition using a string parser how does it understand the difference
> between case (a) and case (b) ? Unless I am missing the point, I think to
> effectively resolve this would need a inverted tree structure for the where
> clause which I think is a bit of an overkill for now ?

Well the code you are adding is primarily a shortcut which is likely
not going to be used by someone at the same time as they manually
author filter strings - so I say we just document the potential issue
and leave it at that. I'm more concerned about the sorting side where
it should be trivial to detect the duplicate.

Speaking of which, this feature should have a paragraph or two in the
docs - I forgot to mention that, sorry (we've been waiting to release
8.3 for months so I'm somewhat rusty at this!).

Thanks, Dave.

On Feb 10, 2008 3:47 PM, Robins Tharakan <tharakan@gmail.com> wrote:
>
> As promised, here are two other patches, one each for the following:
>
>
> 1. IS DISTINCT FROM to replace '<>' in the WHERE conditions
> 2. Documentation addition in the html about the recent Selection/Sort
> changes.
>
> Still working on the crash regarding thread->IsRunning().

I fixed that when I committed your first patch - or did you find
something else wrong?

(will review/commit the rest tomorrow)

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Oracle-compatible database company

On Feb 9, 2008 6:52 PM, Robins Tharakan <tharakan@gmail.com> wrote:
> Hi,
>
> Hmmm... although I did do a complete svn co before submitting my previous
> patch, I think I am making it all complicated by bringing in too many things
> together.
>
> I think you're right. Probably its better to do one thing at a time :)
>
> Attached is a patch to correct the Sort issue. It does an in-place string
> replace if the sort order is changed by the user.
>

Thanks, patch applied!

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Oracle-compatible database company

On Feb 10, 2008 3:47 PM, Robins Tharakan <tharakan@gmail.com> wrote:
>
> As promised, here are two other patches, one each for the following:
>
>
> 1. IS DISTINCT FROM to replace '<>' in the WHERE conditions
>  2. Documentation addition in the html about the recent Selection/Sort
> changes.

Thanks - patches applied!

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com
The Oracle-compatible database company