You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by GitBox <gi...@apache.org> on 2022/11/05 18:35:10 UTC
[GitHub] [netbeans] eirikbakke commented on a diff in pull request #4792: Close whole documents list at once
eirikbakke commented on code in PR #4792:
URL: https://github.com/apache/netbeans/pull/4792#discussion_r1014672995
##########
platform/core.multitabs/src/org/netbeans/core/multitabs/impl/ButtonPopupSwitcher.java:
##########
@@ -355,6 +354,10 @@ private boolean changeSelection( KeyEvent event ) {
selRow = Math.min( pTable.getModel().getRowCount()-1, selRow );
selCol = Math.min( pTable.getModel().getColumnCount()-1, selCol );
switched = true;
+ }else if( tabIndex == -1 ){
Review Comment:
Should have space before "else" and after "if". Some similar formatting errors elsewhere. (Don't worry about fixing formatting issues in existing code, though.)
##########
platform/core.multitabs/src/org/netbeans/core/multitabs/impl/DocumentSwitcherTable.java:
##########
@@ -283,4 +310,4 @@ public boolean isBorderOpaque() {
return false;
}
}
-}
+}
Review Comment:
Unnecessary whitespace change.
##########
platform/core.multitabs/src/org/netbeans/core/multitabs/impl/ButtonPopupSwitcher.java:
##########
@@ -549,4 +552,4 @@ private void selectTab(TabData tab) {
}
}
}
-}
+}
Review Comment:
Unnecessary whitespace change.
##########
platform/core.multitabs/src/org/netbeans/core/multitabs/impl/DocumentSwitcherTable.java:
##########
@@ -174,7 +177,27 @@ public String getToolTipText( MouseEvent event ) {
}
return null;
}
-
+
+ boolean closeSelectedDocumentList() {
Review Comment:
Would be good to add Javadoc explaining what the return value is supposed to mean here.
##########
platform/core.multitabs/src/org/netbeans/core/multitabs/impl/ButtonPopupSwitcher.java:
##########
@@ -374,7 +377,7 @@ private boolean changeSelection( KeyEvent event ) {
}
return switched;
}
-
+
Review Comment:
Unnecessary whitespace change.
##########
platform/core.multitabs/src/org/netbeans/core/multitabs/impl/ButtonPopupSwitcher.java:
##########
@@ -508,7 +511,7 @@ private Item[] createSwitcherItems(final Controller controller) {
}
return items.toArray( new Item[0] );
}
-
+
Review Comment:
Unnecessary whitespace change.
##########
platform/core.multitabs/src/org/netbeans/core/multitabs/impl/DocumentSwitcherTable.java:
##########
@@ -174,7 +177,27 @@ public String getToolTipText( MouseEvent event ) {
}
return null;
}
-
+
+ boolean closeSelectedDocumentList() {
+ List<TabData> tabs = controller.getTabModel().getTabs();
+ Item item = ( Item ) getModel().getValueAt( getSelectedRow(), getSelectedColumn());
+ ProjectProxy project = item.getProject();
+ ProjectSupport projectSupport = ProjectSupport.getDefault();
+ int numOfOtherTabs = 0;
+ for ( int i = tabs.size(); i-- > 0; ) { //reverse iteration to close tabs from the end so tabIndex does not change
+ TabData projectTab = tabs.get( i );
+ ProjectProxy projectForTab = projectSupport.getProjectForTab( projectTab );
+ if (( project == null && projectForTab == null ) || ( projectForTab != null && projectForTab.equals( project ))) {
Review Comment:
Why would the tab be closed in the project == null && projectForTab == null case?
##########
platform/core.multitabs/src/org/netbeans/core/multitabs/impl/DocumentSwitcherTable.java:
##########
@@ -132,26 +132,29 @@ boolean onMouseEvent( MouseEvent e ) {
p = SwingUtilities.convertPoint((Component) e.getSource(), p, this);
int selRow = getSelectedRow();
int selCol = getSelectedColumn();
- if( selRow < 0 || selCol < 0 )
+ if ( selRow < 0 || selCol < 0 )
return false;
Rectangle rect = getCellRect( selRow, selCol, false );
- if( rect.contains( p ) ) {
+ if ( rect.contains( p ) ) {
Dimension size = btnClose.getPreferredSize();
int x = rect.x+rect.width-size.width;
int y = rect.y + (rect.height-size.height)/2;
Rectangle btnRect = new Rectangle( x, y, size.width, size.height);
boolean inButton = btnRect.contains( p );
boolean mustRepaint = inCloseButtonRect != inButton;
inCloseButtonRect = inButton;
- if( inButton ) {
- if( e.getID() == MouseEvent.MOUSE_PRESSED ) {
+ if ( inButton ) {
+ if ( e.getID() == MouseEvent.MOUSE_PRESSED ) {
Item item = ( Item ) getModel().getValueAt( selRow, selCol );
TabData tab = item.getTabData();
+ int tabSize = controller.getTabModel().size();
int tabIndex = controller.getTabModel().indexOf( tab );
if( tabIndex >= 0 ) {
TabActionEvent tae = new TabActionEvent( this, TabbedContainer.COMMAND_CLOSE, tabIndex );
controller.postActionEvent( tae );
- return true;
+ return tabSize == 1;
Review Comment:
What is the purpose of this condition? Under what conditions are onMouseEvent supposed to return true vs. false? (It would be good to add a Javadoc comment explaining the latter.)
##########
platform/core.multitabs/src/org/netbeans/core/multitabs/impl/DocumentSwitcherTable.java:
##########
@@ -174,7 +177,27 @@ public String getToolTipText( MouseEvent event ) {
}
return null;
}
-
+
+ boolean closeSelectedDocumentList() {
+ List<TabData> tabs = controller.getTabModel().getTabs();
+ Item item = ( Item ) getModel().getValueAt( getSelectedRow(), getSelectedColumn());
+ ProjectProxy project = item.getProject();
+ ProjectSupport projectSupport = ProjectSupport.getDefault();
+ int numOfOtherTabs = 0;
+ for ( int i = tabs.size(); i-- > 0; ) { //reverse iteration to close tabs from the end so tabIndex does not change
Review Comment:
Probably better to assemble tabs to be closed in a list first, then iterate over them normally. There may be "Save Changes?" dialogs popping up as a result of trying to close the tabs, and they normally come left-to-right.
##########
platform/core.multitabs/src/org/netbeans/core/multitabs/impl/DocumentSwitcherTable.java:
##########
@@ -27,6 +27,8 @@
import java.awt.Point;
import java.awt.Rectangle;
import java.awt.event.MouseEvent;
+import java.util.Collections;
Review Comment:
Unused import.
##########
platform/core.multitabs/src/org/netbeans/core/multitabs/impl/ButtonPopupSwitcher.java:
##########
@@ -355,6 +354,10 @@ private boolean changeSelection( KeyEvent event ) {
selRow = Math.min( pTable.getModel().getRowCount()-1, selRow );
selCol = Math.min( pTable.getModel().getColumnCount()-1, selCol );
switched = true;
+ }else if( tabIndex == -1 ){
Review Comment:
Why does tabIndex == -1 indicate that closeSelectedDocumentList should be invoked? Would probably be cleaner to check if item.isSeparator(). Nice that you're handling this keypress, though!
##########
platform/core.multitabs/src/org/netbeans/core/multitabs/impl/DocumentSwitcherTable.java:
##########
@@ -174,7 +177,27 @@ public String getToolTipText( MouseEvent event ) {
}
return null;
}
-
+
+ boolean closeSelectedDocumentList() {
+ List<TabData> tabs = controller.getTabModel().getTabs();
+ Item item = ( Item ) getModel().getValueAt( getSelectedRow(), getSelectedColumn());
+ ProjectProxy project = item.getProject();
+ ProjectSupport projectSupport = ProjectSupport.getDefault();
+ int numOfOtherTabs = 0;
+ for ( int i = tabs.size(); i-- > 0; ) { //reverse iteration to close tabs from the end so tabIndex does not change
Review Comment:
Also, there's an off-by-one error here. Reverse for loops over zero-based indices should go "for (int i = N - 1; i >= 0; i--)". (But better iterate left-to-right over a copy of the relevant tabs as suggested above.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists