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

SIGSEGV in Tree.cellDataProc when calling TreeItem.setImage #678 #1662

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -4319,6 +4319,80 @@ void checkSetDataInProcessBeforeRemoval() {
}
}

boolean initPixbufSize(Image image) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] SWT has a ridiculous code style, where parenthesis , braces and brackets are space-separated from associated identifier. Consider following the style of surrounding code.

Copy link
Author

Choose a reason for hiding this comment

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

done: df4c867

if (pixbufSizeSet || image == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] Now that resource management is fixed, are these lines still needed? Can't we monotonically increase the size for every new image?

Copy link
Author

Choose a reason for hiding this comment

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

As I understand from the swt code, for any Tree:

  1. all images in the tree must have the same size
  2. that fixed size is the size of the 1st image set for the tree

In the code this is implemented in TreeItem.setImage(int index, Image image) here:

	if (image != null) {
		ImageList imageList = parent.imageList;
		if (imageList == null) imageList = parent.imageList = new ImageList();
		int imageIndex = imageList.indexOf(image);
		// When we create a blank image surface gets created with dimensions 0, 0.
        // This call recreates the surface with correct dimensions
		long tempSurface = ImageList.convertSurface(image);
		Cairo.cairo_surface_destroy(tempSurface);
		if (imageIndex == -1) {
			imageIndex = imageList.add(image);
		}
		surface = imageList.getSurface(imageIndex);
		pixbuf = ImageList.createPixbuf(surface);
	}

Basically, ImageList has width and height which it initializes from the 1st added image, and then all subsequent added images are scaled to that size.

Here is a snippet that demonstrates how the images in a tree are scaled to the same size:

import org.eclipse.swt.SWT;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.widgets.*;

public class TryColumnsAndImageSizes {

	private Display display;
	private Shell shell;
	private Composite buttonBox;
	private Tree tree;

	private Image[] images = new Image[3];

	public static void main (String[] args) {
		new TryColumnsAndImageSizes ().run ();
	}

	void run () {
		display = new Display ();
		shell = new Shell (display);
		shell.setLayout (new GridLayout ());

		buttonBox = new Composite (shell, 0);
		buttonBox.setLayout (new FillLayout ());

		addSetImageButton ("r50", 0, 20, display.getSystemColor (SWT.COLOR_RED));
		addSetImageButton ("g70", 1, 70, display.getSystemColor (SWT.COLOR_GREEN));
		addSetImageButton ("b90", 2, 120, display.getSystemColor (SWT.COLOR_BLUE));

		tree = new Tree (shell, SWT.VIRTUAL);
		tree.setLayoutData (new GridData (GridData.FILL_BOTH));
		for (int i = 1; i <= 3; i++) {
			var column = new TreeColumn (tree, SWT.LEFT);
			column.setText ("Column " + i);
			column.setWidth (200);
		}

		tree.addListener (SWT.SetData, e -> {
			TreeItem item = (TreeItem) e.item;
			item.setText (0, "a");
			item.setText (1, "b");
			item.setText (2, "c");
			var idx = tree.indexOf (item);
			for (var i = 0; i < images.length; i++) {
				if (idx == i + 2) {
					item.setImage (i, images[i]);
				}
			}
		});
		tree.setItemCount (10);

		shell.setSize (400, 300);
		shell.open ();

		while (!shell.isDisposed ()) {
			if (!display.readAndDispatch ()) {
				display.sleep ();
			}
		}
		display.dispose ();
	}

	private void addSetImageButton (String label, int imageIdx, int imageSize, Color imageColor) {
		var button = new Button (buttonBox, SWT.PUSH);
		button.setText (label);
		button.addListener (SWT.Selection, e -> {
			images[imageIdx] = createImage (imageSize, imageColor);
			tree.clearAll (false);
		});
	}

	private Image createImage (int imageSize, Color color) {
		var image = new Image (display, imageSize, imageSize);
		var gc = new GC (image);
		gc.setForeground (color);
		gc.setBackground (display.getSystemColor (SWT.COLOR_WHITE));
		gc.setLineWidth (5);
		gc.fillRectangle (0, 0, imageSize, imageSize);
		gc.drawRectangle (0, 0, imageSize - 1, imageSize - 1);
		gc.dispose ();
		return image;
	}
}

Copy link
Author

Choose a reason for hiding this comment

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

As a result, the answer to this:

Can't we monotonically increase the size for every new image?

is "no, because Tree in swt is designed to have images of the same size".

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, as long as they are not cropped, we can leave this as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

is "no, because Tree in swt is designed to have images of the same size".

It does not matter, as on each new image we could resize all previously added ones as necessary.

Copy link
Author

Choose a reason for hiding this comment

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

OK.
Apparently, this is another issue.
Just be careful, if you are going to increase fixed row size multiple times, then you'll probably have to replace fixed_height_mode + gtk_tree_view_row_changed to gtk_tree_view_set_model(). Here is more details.

return false;
}
int iWidth, iHeight;
if (DPIUtil.useCairoAutoScale()) {
iWidth = image.getBounds().width;
iHeight = image.getBounds().height;
} else {
iWidth = image.getBoundsInPixels().width;
iHeight = image.getBoundsInPixels().height;
}
if (iWidth <= 0 || iHeight <= 0) {
return false;
}
pixbufSizeSet = true;
pixbufHeight = iHeight;
pixbufWidth = iWidth;
/*
* Feature in GTK: a Tree with the style SWT.VIRTUAL has fixed-height-mode
* enabled. This will limit the size of any cells, including renderers. In order
* to prevent images from disappearing/being cropped, we update row heights when
* the first image is set. Fix for bug 480261.
*/
if ((style & SWT.VIRTUAL) != 0) {
resetFixedRowHeight();
}
return true;
}

private void resetFixedRowHeight() {
long columnList = GTK.gtk_tree_view_get_columns(handle);
if (columnList == 0) {
return;
}

// set fixed width and height for all GtkCellRendererPixbufs
for (var colItem = columnList; colItem != 0; colItem = OS.g_list_next(colItem)) {
long column = OS.g_list_data(colItem);
var cellList = GTK.gtk_cell_layout_get_cells(column);
for (var cellItem = cellList; cellItem != 0; cellItem = OS.g_list_next(cellItem)) {
var renderer = OS.g_list_data(cellItem);
if (GTK.GTK_IS_CELL_RENDERER_PIXBUF(renderer)) {
GTK.gtk_cell_renderer_set_fixed_size(renderer, pixbufWidth, pixbufHeight);
}
}
OS.g_list_free(cellList);
}
OS.g_list_free(columnList);

// Executed in asyncExec because when this method is invoked from checkData(),
// then the "row_changed" signal is blocked (but we need it unblocked to invalidate
// existing rows).
display.asyncExec(() -> {
if (!isDisposed() && modelHandle != 0) {
// reset computed 'fixed_height' to '-1'
OS.g_object_set(handle, OS.fixed_height_mode, false, 0);
OS.g_object_set(handle, OS.fixed_height_mode, true, 0);

// to update height of the existing rows we need to invalidate them in gtk
// we do that by invoking gtk_tree_view_row_changed on each of them
long iter = OS.g_malloc(GTK.GtkTreeIter_sizeof());
if (GTK.gtk_tree_model_get_iter_first(modelHandle, iter)) {
int[] value = new int[1];
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] unnecessary reallocation

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, but I don't understand.
Unnecessary reallocation of what?
As I can see, both iter and value are allocated once.
Could you clarify it, please.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had read int[] value = new int[1]; as being a part of loop iteration. I was wrong. Sorry about that.

Copy link
Author

Choose a reason for hiding this comment

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

no problem

do {
GTK.gtk_tree_model_get(modelHandle, iter, ID_COLUMN, value, -1);
GTK.gtk_tree_store_set(modelHandle, iter, ID_COLUMN, value[0], -1);
} while (GTK.gtk_tree_model_iter_next(modelHandle, iter));
}
OS.g_free(iter);
}
});
}

private void throwCannotRemoveItem(int i) {
String message = "Cannot remove item with index " + i + ".";
throw new SWTException(message);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1519,59 +1519,18 @@ public void setImage(int index, Image image) {
pixbuf = ImageList.createPixbuf(surface);
}

int modelIndex = parent.columnCount == 0 ? Tree.FIRST_COLUMN : parent.columns [index].modelIndex;
long parentHandle = parent.handle;
long column = GTK.gtk_tree_view_get_column (parentHandle, index);
long pixbufRenderer = parent.getPixbufRenderer (column);
int [] currentWidth = new int [1];
int [] currentHeight= new int [1];
GTK.gtk_cell_renderer_get_fixed_size (pixbufRenderer, currentWidth, currentHeight);
if (!parent.pixbufSizeSet) {
if (image != null) {
int iWidth, iHeight;
if (DPIUtil.useCairoAutoScale()) {
iWidth = image.getBounds ().width;
iHeight = image.getBounds ().height;
} else {
iWidth = image.getBoundsInPixels ().width;
iHeight = image.getBoundsInPixels ().height;
}
if (iWidth > currentWidth [0] || iHeight > currentHeight [0]) {
GTK.gtk_cell_renderer_set_fixed_size (pixbufRenderer, iWidth, iHeight);
parent.pixbufSizeSet = true;
parent.pixbufHeight = iHeight;
parent.pixbufWidth = iWidth;
/*
* Feature in GTK: a Tree with the style SWT.VIRTUAL has
* fixed-height-mode enabled. This will limit the size of
* any cells, including renderers. In order to prevent
* images from disappearing/being cropped, we re-create
* the renderers when the first image is set. Fix for
* bug 480261.
*/
if ((parent.style & SWT.VIRTUAL) != 0) {
/*
* Only re-create SWT.CHECK renderers if this is the first column.
* Otherwise check-boxes will be rendered in columns they are not
* supposed to be rendered in. See bug 513761.
*/
boolean check = modelIndex == Tree.FIRST_COLUMN && (parent.style & SWT.CHECK) != 0;
parent.createRenderers(column, modelIndex, check, parent.style);
}
}
}
parent.initPixbufSize (image);
} else {
long column = GTK.gtk_tree_view_get_column (parent.handle, index);
long pixbufRenderer = parent.getPixbufRenderer (column);
/*
* Bug 483112: We check to see if the cached value is greater than the size of the pixbufRenderer.
* If it is, then we change the size of the pixbufRenderer accordingly.
* Bug 489025: There is a corner case where the below is triggered when current(Width|Height) is -1,
* which results in icons being set to 0. Fix is to compare only positive sizes.
* Bug 483112: make sure width and height are set for the pixbufRenderer
*/
if (parent.pixbufWidth > Math.max(currentWidth [0], 0) || parent.pixbufHeight > Math.max(currentHeight [0], 0)) {
GTK.gtk_cell_renderer_set_fixed_size (pixbufRenderer, parent.pixbufWidth, parent.pixbufHeight);
}
GTK.gtk_cell_renderer_set_fixed_size (pixbufRenderer, parent.pixbufWidth, parent.pixbufHeight);
Copy link
Contributor

@basilevs basilevs Dec 13, 2024

Choose a reason for hiding this comment

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

[major] Why do we allow to decrease size now?
Would not this corrupt other columns, where large images might be present?

Copy link
Author

Choose a reason for hiding this comment

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

As I understand, in swt all images in a Tree have the same size. (here is why I think so).
As a result, all GtkCellRendererPixbufs should have the same fixed width and height.

Actually in Tree.resetFixedRowHeight() I added the code that iterates over all GtkCellRendererPixbufs and set them fixed width and height.
So maybe, GTK.gtk_cell_renderer_set_fixed_size() might even be redundant.
I left this code here just in case (e.g. maybe there is code somewhere in swt that adds a new column and doesn't call GTK.gtk_cell_renderer_set_fixed_size() for it)

}

int modelIndex = parent.columnCount == 0 ? Tree.FIRST_COLUMN : parent.columns [index].modelIndex;
GTK.gtk_tree_store_set(parent.modelHandle, handle, modelIndex + Tree.CELL_PIXBUF, pixbuf, -1);
/*
* Bug 573633: gtk_tree_store_set() will reference the handle. So we unref the pixbuf here,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
package org.eclipse.swt.tests.gtk;

import java.util.function.Supplier;

import org.eclipse.swt.SWT;
import org.eclipse.swt.graphics.GC;
import org.eclipse.swt.graphics.Image;
import org.eclipse.swt.layout.GridData;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.swt.widgets.Tree;
import org.eclipse.swt.widgets.TreeItem;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;

public class Test_Gtk_Tree_Virtual_setImage {

private static final int WINDOW_WIDTH = 300;
private static final int WINDOW_HEIGHT = 100;
private static final int IMAGE_SIZE = 70;

private Shell shell;
private Tree tree;
private Image image;

@Before
public void setUp() {
shell = new Shell();
shell.setLayout(new GridLayout());
}

@After
public void tearDown() {
if (tree != null) {
tree.dispose();
tree = null;
}
if (image != null) {
image.dispose();
image = null;
}
if (shell != null) {
shell.dispose();
shell = null;
}
}

@Test
public void test_initial_img_5_2() {
do_test_initial_img(false, 5, 2);
}

@Test
public void test_initial_img_5_2_wCheckbox() {
do_test_initial_img(true, 5, 2);
}

@Test
public void test_initial_img_50_22() {
do_test_initial_img(false, 50, 22);
}

@Test
public void test_initial_img_50_22_wCheckbox() {
do_test_initial_img(true, 50, 22);
}

@Test
public void test_set_img_50_22() {
do_test_set_img(false, 50, 22);
}

@Test
public void test_set_img_50_22_wCheckbox() {
do_test_set_img(true, 50, 22);
}

private void do_test_initial_img(boolean withCheckbox, int totalRows, int imgRowIdx) {
createImage();
createTree(withCheckbox, totalRows, imgRowIdx, () -> image);
shell.pack();
shell.open();

executePendingUiTasks();

tree.showItem(tree.getItem(imgRowIdx));

executePendingUiTasks();

assertAllTreeRowsResized();
}

private void do_test_set_img(boolean withCheckbox, int totalRows, int imgRowIdx) {
createTree(withCheckbox, totalRows, imgRowIdx, () -> image);
shell.pack();
shell.open();

executePendingUiTasks();

tree.showItem(tree.getItem(imgRowIdx));

executePendingUiTasks();

tree.showItem(tree.getItem(totalRows - 1));

executePendingUiTasks();

createImage();

executePendingUiTasks();

tree.clear(imgRowIdx, false);

executePendingUiTasks();

tree.showItem(tree.getItem(imgRowIdx));

executePendingUiTasks();

assertAllTreeRowsResized();
}

private void createImage() {
this.image = new Image(shell.getDisplay(), IMAGE_SIZE, IMAGE_SIZE);
var color = shell.getDisplay().getSystemColor(SWT.COLOR_GREEN);
var gc = new GC(image);
gc.setForeground(color);
gc.setBackground(color);
gc.fillRectangle(0, 0, IMAGE_SIZE, IMAGE_SIZE);
gc.dispose();
}

private void createTree(boolean withCheckbox, int totalRows, int imgRowIdx, Supplier<Image> getImage) {
tree = new Tree(shell, SWT.VIRTUAL | SWT.BORDER | (withCheckbox ? SWT.CHECK : 0));
Copy link
Contributor

@basilevs basilevs Dec 13, 2024

Choose a reason for hiding this comment

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

[minor] Now, that a potential regression for setting an image of decreasing sizes in different columns is discovered, we may want some tests for multi-column case.

Copy link
Author

Choose a reason for hiding this comment

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

It seems to me that Tree doesn't support different images sizes in swt by design.
In this reply I included a snippet that demonstrates what happens when there are multiple columns.
Please, note that images are scaled to fixed size inside ImageList, which is pretty old - that's why I think that Tree expects images of the same size by design.

Copy link
Author

Choose a reason for hiding this comment

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

BTW, I can add unit test(s) for multiple columns and different images sizes to Test_Gtk_Tree_Virtual_setImage no problem.
I just don't know what additional cases you want to be checked.
The current unit tests in Test_Gtk_Tree_Virtual_setImage are for the bug with row heights, that I noticed (and fixed in this PR) while I've been fixing crash in #678.

Copy link
Contributor

Choose a reason for hiding this comment

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

The scenario is:

  • for three sequences of images three in length each:
    • one monotonically increasing in size
    • one monotonically decreasing
    • and one non-monotonic
  • test that behavior (heights, widths, exceptions, ideally cropping but that's probably too hard) is unchanged when:
    • adding images to different columns
    • adding to same column
    • adding to a tree without columns

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear, I'm not a maintainer, so may be wait until one chimes in before investing significant effort. I was going to add such test myself later anyway.

Copy link
Author

Choose a reason for hiding this comment

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

OK.
As I understand, you are working on some task related to image cropping, multiple image sizes, and multiple columns.
I suppose including those unit tests in the commit for this task I would be more appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I literally tried to fix the very same problem - crash in VIRTUAL Tree. While I succeeded, your solution might be a bit better.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I fixed the very same problem - crash in VIRTUAL Tree, but your solution might be better.

var layoutData = new GridData(GridData.FILL_BOTH);
layoutData.widthHint = WINDOW_WIDTH;
layoutData.heightHint = WINDOW_HEIGHT;
tree.setLayoutData(layoutData);
tree.addListener(SWT.SetData, event -> {
var item = (TreeItem) event.item;
var idx = tree.indexOf(item);
item.setText("node " + idx);
if (idx == imgRowIdx) {
var image = getImage.get();
item.setImage(image);
}
item.setItemCount(0);
});
tree.setItemCount(totalRows);
}

private void executePendingUiTasks() {
while (shell.getDisplay().readAndDispatch()) {
// perform next pending task
}
try {
// This sleep is required because
// - gtk perform some updates/renderings on its own inner
// timers (e.g. a timer for every frame drawing)
// - readAndDispatch() doesn't wait for those timers
// - we need to wait for this timers to get fully updated/rendered tree
Thread.sleep(100);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException(e);
}
while (shell.getDisplay().readAndDispatch()) {
// perform next pending task
}
}

private void assertAllTreeRowsResized() {
var minHeight = IMAGE_SIZE;
var maxHeight = 2 * IMAGE_SIZE - 1;
for (var i = 0; i < tree.getItemCount(); i++) {
var item = tree.getItem(i);
var height = item.getBounds().height;
Assert.assertTrue(
String.format("Expected row[%d].height in [%d;%d], but was %d.", i, minHeight, maxHeight, height),
minHeight <= height && height <= maxHeight);
}
}
}
Loading