From 852cd73caaf83b7a2068938e637dd41645814bb1 Mon Sep 17 00:00:00 2001 From: Jonatan Kronqvist Date: Fri, 17 Jun 2011 06:36:12 +0000 Subject: [PATCH] Fix for #6766 - don't repaint the entire table when selecting an item svn changeset:19440/svn branch:6.7 --- src/com/vaadin/terminal/PaintTarget.java | 6 + .../terminal/gwt/client/ui/VScrollTable.java | 35 ++--- .../server/AbstractCommunicationManager.java | 53 +++++--- .../terminal/gwt/server/JsonPaintTarget.java | 9 ++ src/com/vaadin/ui/Table.java | 25 ++-- .../treetable/ProgrammaticSelect.java | 128 ++++++++++++++++++ 6 files changed, 206 insertions(+), 50 deletions(-) create mode 100644 tests/src/com/vaadin/tests/components/treetable/ProgrammaticSelect.java diff --git a/src/com/vaadin/terminal/PaintTarget.java b/src/com/vaadin/terminal/PaintTarget.java index d118562588..b38b65e8dc 100644 --- a/src/com/vaadin/terminal/PaintTarget.java +++ b/src/com/vaadin/terminal/PaintTarget.java @@ -474,4 +474,10 @@ public interface PaintTarget extends Serializable { * paintable. */ public String getTag(Paintable paintable); + + /** + * @return true if a full repaint has been requested. E.g. refresh in a + * browser window or such. + */ + public boolean isFullRepaint(); } diff --git a/src/com/vaadin/terminal/gwt/client/ui/VScrollTable.java b/src/com/vaadin/terminal/gwt/client/ui/VScrollTable.java index 381e9c83b4..a0feaddcad 100644 --- a/src/com/vaadin/terminal/gwt/client/ui/VScrollTable.java +++ b/src/com/vaadin/terminal/gwt/client/ui/VScrollTable.java @@ -863,25 +863,26 @@ public class VScrollTable extends FlowPanel implements Table, ScrollHandler, addRowsToBody(partialRowAdditions); } else { UIDL rowData = uidl.getChildByTagName("rows"); - if (!recalcWidths && initializedAndAttached) { - updateBody(rowData, uidl.getIntAttribute("firstrow"), - uidl.getIntAttribute("rows")); - if (headerChangedDuringUpdate) { - lazyAdjustColumnWidths.schedule(1); + if (rowData != null) { + if (!recalcWidths && initializedAndAttached) { + updateBody(rowData, uidl.getIntAttribute("firstrow"), + uidl.getIntAttribute("rows")); + if (headerChangedDuringUpdate) { + lazyAdjustColumnWidths.schedule(1); + } else { + // webkits may still bug with their disturbing scrollbar + // bug, see #3457 + // Run overflow fix for the scrollable area + Scheduler.get().scheduleDeferred(new Command() { + public void execute() { + Util.runWebkitOverflowAutoFix(scrollBodyPanel + .getElement()); + } + }); + } } else { - // webkits may still bug with their disturbing scrollbar - // bug, - // See #3457 - // run overflow fix for scrollable area - Scheduler.get().scheduleDeferred(new Command() { - public void execute() { - Util.runWebkitOverflowAutoFix(scrollBodyPanel - .getElement()); - } - }); + initializeRows(uidl, rowData); } - } else { - initializeRows(uidl, rowData); } } diff --git a/src/com/vaadin/terminal/gwt/server/AbstractCommunicationManager.java b/src/com/vaadin/terminal/gwt/server/AbstractCommunicationManager.java index 74b7a27a96..e3dd014dec 100644 --- a/src/com/vaadin/terminal/gwt/server/AbstractCommunicationManager.java +++ b/src/com/vaadin/terminal/gwt/server/AbstractCommunicationManager.java @@ -682,15 +682,14 @@ public abstract class AbstractCommunicationManager implements requestThemeName = request.getParameter("theme"); maxInactiveInterval = request.getSession().getMaxInactiveInterval(); // repaint requested or session has timed out and new one is created - boolean repaintAll; final OutputStream out; - repaintAll = (request.getParameter(GET_PARAM_REPAINT_ALL) != null); + setRepaintAll(request.getParameter(GET_PARAM_REPAINT_ALL) != null); // || (request.getSession().isNew()); FIXME What the h*ll is this?? out = response.getOutputStream(); boolean analyzeLayouts = false; - if (repaintAll) { + if (isRepaintAll()) { // analyzing can be done only with repaintAll analyzeLayouts = (request.getParameter(GET_PARAM_ANALYZE_LAYOUTS) != null); } @@ -748,12 +747,11 @@ public abstract class AbstractCommunicationManager implements } } // No message to show, let's just repaint all. - repaintAll = true; - + setRepaintAll(true); } - paintAfterVariableChanges(request, response, callback, repaintAll, - outWriter, window, analyzeLayouts); + paintAfterVariableChanges(request, response, callback, outWriter, + window, analyzeLayouts); if (closingWindowName != null) { currentlyOpenWindowsInClient.remove(closingWindowName); @@ -771,7 +769,6 @@ public abstract class AbstractCommunicationManager implements * @param request * @param response * @param callback - * @param repaintAll * @param outWriter * @param window * @param analyzeLayouts @@ -779,11 +776,10 @@ public abstract class AbstractCommunicationManager implements * @throws IOException */ private void paintAfterVariableChanges(Request request, Response response, - Callback callback, boolean repaintAll, final PrintWriter outWriter, - Window window, boolean analyzeLayouts) throws PaintException, - IOException { + Callback callback, final PrintWriter outWriter, Window window, + boolean analyzeLayouts) throws PaintException, IOException { - if (repaintAll) { + if (isRepaintAll()) { makeAllPaintablesDirty(window); } @@ -823,11 +819,10 @@ public abstract class AbstractCommunicationManager implements application, window); if (newWindow != window) { window = newWindow; - repaintAll = true; + setRepaintAll(true); } - writeUidlResponce(callback, repaintAll, outWriter, window, - analyzeLayouts); + writeUidlResponse(callback, outWriter, window, analyzeLayouts); } closeJsonMessage(outWriter); @@ -836,7 +831,7 @@ public abstract class AbstractCommunicationManager implements } - public void writeUidlResponce(Callback callback, boolean repaintAll, + public void writeUidlResponse(Callback callback, final PrintWriter outWriter, Window window, boolean analyzeLayouts) throws PaintException { outWriter.print("\"changes\":["); @@ -846,7 +841,7 @@ public abstract class AbstractCommunicationManager implements List invalidComponentRelativeSizes = null; JsonPaintTarget paintTarget = new JsonPaintTarget(this, outWriter, - !repaintAll); + !isRepaintAll()); OpenWindowCache windowCache = currentlyOpenWindowsInClient.get(window .getName()); if (windowCache == null) { @@ -855,7 +850,7 @@ public abstract class AbstractCommunicationManager implements } // Paints components - if (repaintAll) { + if (isRepaintAll()) { paintables = new ArrayList(); paintables.add(window); @@ -970,7 +965,7 @@ public abstract class AbstractCommunicationManager implements outWriter.print(", \"meta\" : {"); boolean metaOpen = false; - if (repaintAll) { + if (isRepaintAll()) { metaOpen = true; outWriter.write("\"repaintAll\":true"); if (analyzeLayouts) { @@ -1016,7 +1011,7 @@ public abstract class AbstractCommunicationManager implements && ci.getSessionExpiredCaption() == null && ci.isSessionExpiredNotificationEnabled()) { int newTimeoutInterval = getTimeoutInterval(); - if (repaintAll || (timeoutInterval != newTimeoutInterval)) { + if (isRepaintAll() || (timeoutInterval != newTimeoutInterval)) { String escapedURL = ci.getSessionExpiredURL() == null ? "" : ci .getSessionExpiredURL().replace("/", "\\/"); if (metaOpen) { @@ -1107,6 +1102,7 @@ public abstract class AbstractCommunicationManager implements if (dragAndDropService != null) { dragAndDropService.printJSONResponse(outWriter); } + setRepaintAll(false); } private int getTimeoutInterval() { @@ -1219,8 +1215,9 @@ public abstract class AbstractCommunicationManager implements final PrintWriter outWriter = new PrintWriter( new CharArrayWriter()); + setRepaintAll(true); paintAfterVariableChanges(request, response, callback, - true, outWriter, window, false); + outWriter, window, false); } @@ -2122,6 +2119,8 @@ public abstract class AbstractCommunicationManager implements private final HashMap, Integer> typeToKey = new HashMap, Integer>(); private int nextTypeKey = 0; + private boolean repaintAll = false; + String getTagForType(Class class1) { Integer object = typeToKey.get(class1); if (object == null) { @@ -2311,4 +2310,16 @@ public abstract class AbstractCommunicationManager implements return b; } } + + private void setRepaintAll(boolean repaintAll) { + this.repaintAll = repaintAll; + } + + /** + * @return true if a RepaintAll has been requested. E.g. by refreshing the + * browser window or such. + */ + public boolean isRepaintAll() { + return repaintAll; + } } diff --git a/src/com/vaadin/terminal/gwt/server/JsonPaintTarget.java b/src/com/vaadin/terminal/gwt/server/JsonPaintTarget.java index 8b1e776b4c..da7f838bda 100644 --- a/src/com/vaadin/terminal/gwt/server/JsonPaintTarget.java +++ b/src/com/vaadin/terminal/gwt/server/JsonPaintTarget.java @@ -1111,4 +1111,13 @@ public class JsonPaintTarget implements PaintTarget { } + /* + * (non-Javadoc) + * + * @see com.vaadin.terminal.PaintTarget#isFullRepaint() + */ + public boolean isFullRepaint() { + return manager.isRepaintAll(); + } + } diff --git a/src/com/vaadin/ui/Table.java b/src/com/vaadin/ui/Table.java index 907b3a98d9..dfa4aad005 100644 --- a/src/com/vaadin/ui/Table.java +++ b/src/com/vaadin/ui/Table.java @@ -393,6 +393,8 @@ public class Table extends AbstractSelect implements Action.Container, private MultiSelectMode multiSelectMode = MultiSelectMode.DEFAULT; + private boolean rowCacheInvalidated; + /* Table constructors */ /** @@ -1471,6 +1473,7 @@ public class Table extends AbstractSelect implements Action.Container, pageBufferFirstIndex = firstIndex; } + setRowCacheInvalidated(true); requestRepaint(); } @@ -1820,17 +1823,6 @@ public class Table extends AbstractSelect implements Action.Container, return itemId; } - /* Overriding select behavior */ - - @Override - public void setValue(Object newValue) throws ReadOnlyException, - ConversionException { - // external selection change, need to truncate pageBuffer - resetPageBuffer(); - refreshRenderedCells(); - super.setValue(newValue); - } - @Override public void setContainerDataSource(Container newDataSource) { @@ -2326,8 +2318,9 @@ public class Table extends AbstractSelect implements Action.Container, // Rows if (isPartialRowUpdate()) { paintPartialRowUpdate(target, actionSet); - } else { + } else if (target.isFullRepaint() || isRowCacheInvalidated()) { paintRows(target, cells, actionSet); + setRowCacheInvalidated(false); } paintSorting(target); @@ -2349,6 +2342,14 @@ public class Table extends AbstractSelect implements Action.Container, } } + private void setRowCacheInvalidated(boolean invalidated) { + rowCacheInvalidated = invalidated; + } + + private boolean isRowCacheInvalidated() { + return rowCacheInvalidated; + } + private void paintPartialRowUpdate(PaintTarget target, Set actionSet) throws PaintException { paintPartialRowUpdates(target, actionSet); diff --git a/tests/src/com/vaadin/tests/components/treetable/ProgrammaticSelect.java b/tests/src/com/vaadin/tests/components/treetable/ProgrammaticSelect.java new file mode 100644 index 0000000000..fc68d0be15 --- /dev/null +++ b/tests/src/com/vaadin/tests/components/treetable/ProgrammaticSelect.java @@ -0,0 +1,128 @@ +package com.vaadin.tests.components.treetable; + +import java.util.ArrayList; +import java.util.List; + +import com.vaadin.data.Container; +import com.vaadin.data.Item; +import com.vaadin.data.util.HierarchicalContainer; +import com.vaadin.tests.components.TestBase; +import com.vaadin.ui.Button; +import com.vaadin.ui.Button.ClickEvent; +import com.vaadin.ui.TreeTable; + +public class ProgrammaticSelect extends TestBase { + + @Override + protected void setup() { + final TreeTable tt = new TreeTable(); + tt.setContainerDataSource(buildDataSource(10, 100, 50)); + tt.setSelectable(true); + addComponent(tt); + + Button selectItem = new Button("Select first row", + new Button.ClickListener() { + + public void buttonClick(ClickEvent event) { + Object id = tt.getItemIds().iterator().next(); + tt.select(id); + } + }); + + addComponent(selectItem); + } + + private Container buildDataSource(int properties, int items, int roots) { + Container.Hierarchical c = new HierarchicalContainer(); + + populateContainer(c, properties, items); + + if (items <= roots) { + return c; + } + + // "roots" roots, each with + // "firstLevel" children, two with no children (one with childAllowed, + // one without) + // ("firstLevel"-2)*"secondLevel" children ("secondLevel"/2 with + // childAllowed, "secondLevel"/2 without) + + // N*M+N*(M-2)*C = items + // items=N(M+MC-2C) + + // Using secondLevel=firstLevel/2 => + // items = roots*(firstLevel+firstLevel*firstLevel/2-2*firstLevel/2) + // =roots*(firstLevel+firstLevel^2/2-firstLevel) + // = roots*firstLevel^2/2 + // => firstLevel = sqrt(items/roots*2) + + int firstLevel = (int) Math.ceil(Math.sqrt(items / roots * 2.0)); + int secondLevel = firstLevel / 2; + + while (roots * (1 + 2 + (firstLevel - 2) * secondLevel) < items) { + // Increase something so we get enough items + secondLevel++; + } + + List itemIds = new ArrayList(c.getItemIds()); + + int nextItemId = roots; + for (int rootIndex = 0; rootIndex < roots; rootIndex++) { + // roots use items 0..roots-1 + Object rootItemId = itemIds.get(rootIndex); + + // force roots to be roots even though they automatically should be + c.setParent(rootItemId, null); + + for (int firstLevelIndex = 0; firstLevelIndex < firstLevel; firstLevelIndex++) { + if (nextItemId >= items) { + break; + } + Object firstLevelItemId = itemIds.get(nextItemId++); + c.setParent(firstLevelItemId, rootItemId); + + if (firstLevelIndex < 2) { + continue; + } + + // firstLevelChildren 2.. have child nodes + for (int secondLevelIndex = 0; secondLevelIndex < secondLevel; secondLevelIndex++) { + if (nextItemId >= items) { + break; + } + + Object secondLevelItemId = itemIds.get(nextItemId++); + c.setParent(secondLevelItemId, firstLevelItemId); + } + } + } + + return c; + } + + private void populateContainer(Container c, int properties, int items) { + c.removeAllItems(); + for (int i = 1; i <= properties; i++) { + c.addContainerProperty("Property " + i, String.class, ""); + } + for (int i = 1; i <= items; i++) { + Item item = c.addItem("Item " + i); + for (int j = 1; j <= properties; j++) { + item.getItemProperty("Property " + j).setValue( + "Item " + i + "," + j); + } + } + + } + + @Override + protected String getDescription() { + return "Programmatically selecting an item should not cause a complete repaint"; + } + + @Override + protected Integer getTicketNumber() { + return 6766; + } + +} -- 2.39.5