#15077 closed enhancement (fixed)

Allow customizing grid column autosizing

Reported by: oneeyeman Owned by: vadz
Priority: low Milestone:
Component: wxGrid Version: stable-latest
Keywords: autosize Cc:
Blocked By: Blocking:
Patch: yes

Description

Currently when you try to autosize the column in the grid it will be resized by the width of the column label.
This patch makes it possible to autosize the column by the the width of the widest string in the column.

This will bring more consistency with Excel while preserving compatibility.

Please review and apply.

Patch is created based on the SVN rev. 73512

Attachments (1)

grid_autosize.patch download (6.8 KB) - added by oneeyeman 20 months ago.
Implementation patch

Download all attachments as: .zip

Change History (13)

comment:2 Changed 21 months ago by vadz

  • Keywords simple removed
  • Priority changed from normal to low
  • Status changed from new to confirmed
  • Summary changed from Grid column autosizing to Allow customizing grid column autosizing

The new event is called COL_AUTO_SIZE in the docs but AUTO_COL_SIZE in the code, need to choose one (I prefer the former).

Also, why move the event class declaration? Just forward declare it at the top of the file.

comment:3 Changed 21 months ago by oneeyeman

Vadim,
All issues should be corrected.
Also sample shows one possible implementation of this feature.

Just remembered: forgot to add @since to the docs. Could you please fix it when applying?
Don't want to reboot back in Windows just for that...

Thank you.

comment:4 follow-up: Changed 20 months ago by vadz

Unfortunately there are still some points in this patch that need to be changed:

  1. I don't understand the reason for removing of the code sending wxEVT_GRID_COL_SIZE event. Was this accidental?
  2. Why is the parent window used in wxGrid::OnAutoSizeColumn()? I don't understand at all what are you trying to do here... It would really make much more sense to send the event to the grid itself and call AutoSizeColLabelSize() only if the event was not processed.
  3. The sample changes are also incomprehensible (to me at least). You're filling a hash map with the sizes of all columns and then you only ever use exactly one of its elements? Just what exactly are you trying to do? If you could demonstrate a normal use of this event, e.g. really go through all the values in this column and computing their size and taking the max of this, it would be much more useful.
  4. Please mention in the documentation that the event is new since 2.9.5 (you can't use "@since" in inline paragraphs so just write it in full).
  5. As usual, there is some strange indentation (just look at the diff in Trac), probably due to either hard TABs or the TABs being replaced with 8 spaces instead of 4.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 20 months ago by oneeyeman

Vadim,
Replying to vadz:

Unfortunately there are still some points in this patch that need to be changed:

  1. I don't understand the reason for removing of the code sending wxEVT_GRID_COL_SIZE event. Was this accidental?

What I'm trying to do here is to bring the grid to behave like the normal spreadsheet (i.e. Excel). If you double click on the column divider the column to the left become auto sized. If you do DnD of this divider then the column on the left will be re-sized. I think that that piece of code was wrong and with introduction of this "column auto size" event it will behave more like an Excel (or any spreadsheet). I understand that it's possible I'm breaking user code here, but I am making the code more consistent with existing applications. If you still disagree with this point let me know. I will try to keep the old behavior if possible.

  1. Why is the parent window used in wxGrid::OnAutoSizeColumn()? I don't understand at all what are you trying to do here... It would really make much more sense to send the event to the grid itself and call AutoSizeColLabelSize() only if the event was not processed.

I don't remember. I think I did it by the analogy with some other code.
Anyway you suggestion make sense and I will re-check if it works.

  1. The sample changes are also incomprehensible (to me at least). You're filling a hash map with the sizes of all columns and then you only ever use exactly one of its elements? Just what exactly are you trying to do? If you could demonstrate a normal use of this event, e.g. really go through all the values in this column and computing their size and taking the max of this, it would be much more useful.

As you said yourself, computing the size of the column will be very slow. I tested this and it was ugly. So instead I introduce a hash that will keep the column sizes that is set when the grid is auto sized. This implementation is much faster. It probably would be more useful if I don't use the hash just for one column, but it's a remnant of the previous test. I will remove the condition and resubmit if my explanation is not understood.

  1. Please mention in the documentation that the event is new since 2.9.5 (you can't use "@since" in inline paragraphs so just write it in full).

OK, will do.

  1. As usual, there is some strange indentation (just look at the diff in Trac), probably due to either hard TABs or the TABs being replaced with 8 spaces instead of 4.

Ouch. That's gotta hurt. ;-)
I will fix the spacing.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 20 months ago by vadz

Replying to oneeyeman:

Vadim,
Replying to vadz:

Unfortunately there are still some points in this patch that need to be changed:

  1. I don't understand the reason for removing of the code sending wxEVT_GRID_COL_SIZE event. Was this accidental?

What I'm trying to do here is to bring the grid to behave like the normal spreadsheet (i.e. Excel). If you double click on the column divider the column to the left become auto sized. If you do DnD of this divider then the column on the left will be re-sized. I think that that piece of code was wrong and with introduction of this "column auto size" event it will behave more like an Excel (or any spreadsheet). I understand that it's possible I'm breaking user code here, but I am making the code more consistent with existing applications. If you still disagree with this point let me know. I will try to keep the old behavior if possible.

I don't see the problem. Just generate the new autosize event first to let the user code to compute the best column size differently but continue sending the "col size changed" notification anyhow. Why not?

  1. The sample changes are also incomprehensible (to me at least). You're filling a hash map with the sizes of all columns and then you only ever use exactly one of its elements? Just what exactly are you trying to do? If you could demonstrate a normal use of this event, e.g. really go through all the values in this column and computing their size and taking the max of this, it would be much more useful.

As you said yourself, computing the size of the column will be very slow.

I said it could be very slow with very large grids, which is why we don't want to do it unconditionally. But it should be instantaneous with a 100 row grid.

I tested this and it was ugly.

Something must be wrong here. Doing 100 calls to GetTextExtent() shouldn't be noticeable at all (especially if you optimize away the calls for the empty strings).

comment:7 in reply to: ↑ 6 ; follow-up: Changed 20 months ago by oneeyeman

Vadim,

Replying to vadz:

Replying to oneeyeman:

Vadim,
Replying to vadz:

Unfortunately there are still some points in this patch that need to be changed:

  1. I don't understand the reason for removing of the code sending wxEVT_GRID_COL_SIZE event. Was this accidental?

What I'm trying to do here is to bring the grid to behave like the normal spreadsheet (i.e. Excel). If you double click on the column divider the column to the left become auto sized. If you do DnD of this divider then the column on the left will be re-sized. I think that that piece of code was wrong and with introduction of this "column auto size" event it will behave more like an Excel (or any spreadsheet). I understand that it's possible I'm breaking user code here, but I am making the code more consistent with existing applications. If you still disagree with this point let me know. I will try to keep the old behavior if possible.

I don't see the problem. Just generate the new autosize event first to let the user code to compute the best column size differently but continue sending the "col size changed" notification anyhow. Why not?

But then user will need to process 2 messages instead of one, no?
OTOH, I see you point here. The column size changed so the other notification should be sent. I will see how it will work.

  1. The sample changes are also incomprehensible (to me at least). You're filling a hash map with the sizes of all columns and then you only ever use exactly one of its elements? Just what exactly are you trying to do? If you could demonstrate a normal use of this event, e.g. really go through all the values in this column and computing their size and taking the max of this, it would be much more useful.

As you said yourself, computing the size of the column will be very slow.

I said it could be very slow with very large grids, which is why we don't want to do it unconditionally. But it should be instantaneous with a 100 row grid.

Well this implementation is good enough for all grids. And there will be no conditionals.

comment:8 in reply to: ↑ 7 Changed 20 months ago by vadz

Replying to oneeyeman:

Vadim,

Replying to vadz:

I don't see the problem. Just generate the new autosize event first to let the user code to compute the best column size differently but continue sending the "col size changed" notification anyhow. Why not?

But then user will need to process 2 messages instead of one, no?

No. They will be able to handle both events if necessary but there is no "need" to process both of them. It would be necessary to explain this clearly in the documentation, of course, if this is really as confusing as it seems to be...

I said it could be very slow with very large grids, which is why we don't want to do it unconditionally. But it should be instantaneous with a 100 row grid.

Well this implementation is good enough for all grids. And there will be no conditionals.

The code in the sample is supposed to show to wx users how to do things. This code doesn't show anything because it doesn't make any sense. And you don't care about the performance of autosizing for huge grids for the simple reason that you won't use it with them!

Changed 20 months ago by oneeyeman

Implementation patch

comment:9 Changed 20 months ago by oneeyeman

Vadim,
Attached is the next version. Hopefully everything is addressed.

Thank you for the review.

comment:10 Changed 20 months ago by vadz

  • Owner set to vadz
  • Status changed from confirmed to accepted

I just don't understand how is it possible that each new version of the patch introduces its own new problems :-( This one has changed the default behaviour of wxGrid auto-sizing to fit the column to its contents, which wasn't done by the previous versions. Why??

Anyhow, I'll fix it and apply this patch but, Igot, I really, really don't have enough time to spend so much on it on such simple patches. Please try to test/debug/review them more yourself. I appreciate your help but at this stage it would have been faster for me to implement this myself from the beginning than review this patch so many times and this is, unfortunately, not an isolated example.

comment:11 Changed 20 months ago by vadz

P.S. And handling the event in the sample plainly doesn't work at all :-( Have you really tested it?

comment:12 Changed 20 months ago by VZ

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [73789]) Allow to customize wxGrid column auto-sizing.

By default the columns are auto-sized to fit just their label, which is fast
but not very user-friendly. Allow customizing this behaviour by handling the
(new) wxEVT_GRID_COL_AUTO_SIZE event.

Closes #15077.

Note: See TracTickets for help on using tickets.