From 739ffcd62640358c05d29b3f8e132a48823f0bcf Mon Sep 17 00:00:00 2001
From: Matthijs Kooijman <matthijs@stdin.nl>
Date: Tue, 1 Oct 2019 12:48:34 +0200
Subject: [PATCH 1/3] Remove unneeded color-setting code in the boards and
 library manager

Previously, for the boards manager:
 - InstallerJDialog would set the "selection background" color on the
   table, using the "status.notice.bgcolor" the color (default blueish
   green). This color is not used directly, but made available for cell
   renderers to use.
   https://github.com/arduino/Arduino/blob/a1448876a1115c9d3ee9e88f29a15bb081a27816/app/src/cc/arduino/contributions/ui/InstallerJDialog.java#L183
 - For each cell, either a ContributedPlatformTableCellEditor or
   ContributedPlatformTableCellRenderer is used, depending on whether
   the cell is being edited (i.e. when selected).
 - Both of these create a ContributedPlatformTableCellJPanel, and call
   its `update` method, which creates the components for the cell.
 - The `update` method als sets the background color of the description
   to white, which does not actually have any effect because the
   description is not opaque.
   https://github.com/arduino/Arduino/blob/a1448876a1115c9d3ee9e88f29a15bb081a27816/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java#L271
   https://github.com/arduino/Arduino/blob/a1448876a1115c9d3ee9e88f29a15bb081a27816/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java#L309
   https://docs.oracle.com/javase/7/docs/api/javax/swing/JComponent.html#setBackground(java.awt.Color)
 - The `update` method also sets its colors of itself (JPanel) to the FG
   and BG color, or the selected FG and BG color of the table depending
   on the selected status of the cell. These seem to default to black on
   white for non-selected and white on blue-ish for selected cells.
   However, InstallJDialog has replaced the selected BG with a blueish
   green, as shown above.
   Of these, only the BG colors actually seem to take effect. The fg
   color of the description component is actually used (default black).
   https://github.com/arduino/Arduino/blob/a1448876a1115c9d3ee9e88f29a15bb081a27816/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java#L282-L288
 - After calling `update`, ContributedPlatformTableCellEditor overrides
   the JPanel background color with a fixed grey color. Similarly,
   ContributedPlatformTableCellRenderer sets an alternating white and
   (slightly lighter) grey background color. Together, this means that
   the background color set by ContributedPlatformTableCellJPanel is
   never actually used.
   https://github.com/arduino/Arduino/blob/a1448876a1115c9d3ee9e88f29a15bb081a27816/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellEditor.java#L132-L133
   https://github.com/arduino/Arduino/blob/a1448876a1115c9d3ee9e88f29a15bb081a27816/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellRenderer.java#L47-L53

For the library manager, pretty much the same happens.

Effectively, the only colors that were actually used were the background
colors set by ContributedPlatformTableCellEditor and
ContributedPlatformTableCellRenderer. This is problematic because:
 - There is a lot of other confusing and unused code
 - The foreground color is never set. This is fine when it is black or
   another dark color, but when the system is configured with a dark
   theme, the default foreground color will be white, which is
   problematic on a white background.

This commit remove the unneeded code, setting the foreground color
is left for later. It also removes the (now unused) `isSelected` from
`ContributedPlatformTableCellJPanel::update`. For the library manager,
the corresponding argument is still used to decide the "author" color.
---
 .../ui/ContributedLibraryTableCellJPanel.java        |  9 ---------
 .../ui/ContributedPlatformTableCellEditor.java       |  2 +-
 .../ui/ContributedPlatformTableCellJPanel.java       | 12 +-----------
 .../ui/ContributedPlatformTableCellRenderer.java     |  2 +-
 .../arduino/contributions/ui/InstallerJDialog.java   |  1 -
 5 files changed, 3 insertions(+), 23 deletions(-)

diff --git a/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellJPanel.java b/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellJPanel.java
index 865de577630..a6ce80d86de 100644
--- a/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellJPanel.java
+++ b/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellJPanel.java
@@ -205,7 +205,6 @@ public ContributedLibraryTableCellJPanel(JTable parentTable, Object value,
     description.setText(desc);
     // copy description to accessibility context for screen readers to use
     description.getAccessibleContext().setAccessibleDescription(desc);
-    description.setBackground(Color.WHITE);
 
     // for modelToView to work, the text area has to be sized. It doesn't
     // matter if it's visible or not.
@@ -215,14 +214,6 @@ public ContributedLibraryTableCellJPanel(JTable parentTable, Object value,
     InstallerTableCell
         .setJTextPaneDimensionToFitContainedText(description,
                                                  parentTable.getBounds().width);
-
-    if (isSelected) {
-      setBackground(parentTable.getSelectionBackground());
-      setForeground(parentTable.getSelectionForeground());
-    } else {
-      setBackground(parentTable.getBackground());
-      setForeground(parentTable.getForeground());
-    }
   }
 
   // same function as in ContributedPlatformTableCellJPanel - is there a utils file this can move to?
diff --git a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellEditor.java b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellEditor.java
index 26bcb1c9d3b..acfb58bb3ea 100644
--- a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellEditor.java
+++ b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellEditor.java
@@ -129,7 +129,7 @@ public Component getTableCellEditorComponent(JTable table, Object _value,
     cell.versionToInstallChooser
         .setVisible(installed == null && uninstalledReleases.size() > 1);
 
-    cell.update(table, _value, true, !installedBuiltIn.isEmpty());
+    cell.update(table, _value, !installedBuiltIn.isEmpty());
     cell.setBackground(new Color(218, 227, 227)); // #dae3e3
     return cell;
   }
diff --git a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java
index d59538823f7..d20d708530c 100644
--- a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java
+++ b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java
@@ -184,8 +184,7 @@ private String setButtonOrLink(JButton button, String desc, String label, String
     return retString;
   }
 
-  void update(JTable parentTable, Object value, boolean isSelected,
-              boolean hasBuiltInRelease) {
+  void update(JTable parentTable, Object value, boolean hasBuiltInRelease) {
     ContributedPlatformReleases releases = (ContributedPlatformReleases) value;
 
     JTextPane description = makeNewDescription();
@@ -268,7 +267,6 @@ void update(JTable parentTable, Object value, boolean isSelected,
     description.setText(desc);
     // copy description to accessibility context for screen readers to use
     description.getAccessibleContext().setAccessibleDescription(desc);
-    description.setBackground(Color.WHITE);
 
     // for modelToView to work, the text area has to be sized. It doesn't
     // matter if it's visible or not.
@@ -278,14 +276,6 @@ void update(JTable parentTable, Object value, boolean isSelected,
     int width = parentTable.getBounds().width;
     InstallerTableCell.setJTextPaneDimensionToFitContainedText(description,
                                                                width);
-
-    if (isSelected) {
-      setBackground(parentTable.getSelectionBackground());
-      setForeground(parentTable.getSelectionForeground());
-    } else {
-      setBackground(parentTable.getBackground());
-      setForeground(parentTable.getForeground());
-    }
   }
 
   private JTextPane makeNewDescription() {
diff --git a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellRenderer.java b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellRenderer.java
index cc4b1d0c186..19d484dc23e 100644
--- a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellRenderer.java
+++ b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellRenderer.java
@@ -44,7 +44,7 @@ public Component getTableCellRendererComponent(JTable table, Object value,
                                                  int column) {
     ContributedPlatformTableCellJPanel cell = new ContributedPlatformTableCellJPanel();
     cell.setButtonsVisible(false);
-    cell.update(table, value, isSelected, false);
+    cell.update(table, value, false);
 
     if (row % 2 == 0) {
       cell.setBackground(new Color(236, 241, 241)); // #ecf1f1
diff --git a/app/src/cc/arduino/contributions/ui/InstallerJDialog.java b/app/src/cc/arduino/contributions/ui/InstallerJDialog.java
index dd2998bc441..2888cd68800 100644
--- a/app/src/cc/arduino/contributions/ui/InstallerJDialog.java
+++ b/app/src/cc/arduino/contributions/ui/InstallerJDialog.java
@@ -180,7 +180,6 @@ public void windowOpened(WindowEvent e) {
     contribTable.setDragEnabled(false);
     contribTable.setIntercellSpacing(new Dimension(0, 1));
     contribTable.setShowVerticalLines(false);
-    contribTable.setSelectionBackground(Theme.getColor("status.notice.bgcolor"));
     contribTable.addKeyListener(new AbstractKeyListener() {
 
       @Override

From b1f954d5dcd794858f8903a3242216a6e85aee92 Mon Sep 17 00:00:00 2001
From: Matthijs Kooijman <matthijs@stdin.nl>
Date: Tue, 1 Oct 2019 14:57:28 +0200
Subject: [PATCH 2/3] In the board/library manager, create the description
 component only once

Previously,`makeNewDescription` was called in the constructor and then
again later in the `update` method (board manager) or later in the
constructor (library manager) to recreate the description JTextPane so
it can be filled with text. In all cases, the pane would be created
equal, so there is no point in recreating it.

Now, it is created only once and stored in an instance variable for
later reference. Additionally, `makeNewDescription` now only creates the
JTextPane, the constructor handles adding it (like for other
components).

This change slightly simplifies code, but also prepares for allowing
to change the description text color externally in a later commit.

For the library manager it is not currently strictly needed to have an
instance variable (since the description is only used inside the
constructor), but the instance variable is added for consistency and to
prepare for this same upcoming change.
---
 .../ui/ContributedLibraryTableCellJPanel.java          |  9 +++------
 .../ui/ContributedPlatformTableCellJPanel.java         | 10 +++-------
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellJPanel.java b/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellJPanel.java
index a6ce80d86de..cbef1ba3870 100644
--- a/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellJPanel.java
+++ b/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellJPanel.java
@@ -42,6 +42,7 @@ public class ContributedLibraryTableCellJPanel extends JPanel {
   final JPanel buttonsPanel;
   final JPanel inactiveButtonsPanel;
   final JLabel statusLabel;
+  final JTextPane description;
   private final String moreInfoLbl = tr("More info");
 
   public ContributedLibraryTableCellJPanel(JTable parentTable, Object value,
@@ -77,7 +78,8 @@ public ContributedLibraryTableCellJPanel(JTable parentTable, Object value,
     versionToInstallChooser
         .setMinimumSize(new Dimension((int)versionToInstallChooser.getPreferredSize().getWidth() + 50, (int)versionToInstallChooser.getPreferredSize().getHeight()));
 
-    makeNewDescription();
+    description = makeNewDescription();
+    add(description);
 
     buttonsPanel = new JPanel();
     buttonsPanel.setLayout(new BoxLayout(buttonsPanel, BoxLayout.X_AXIS));
@@ -121,7 +123,6 @@ public ContributedLibraryTableCellJPanel(JTable parentTable, Object value,
     add(Box.createVerticalStrut(15));
 
     ContributedLibraryReleases releases = (ContributedLibraryReleases) value;
-    JTextPane description = makeNewDescription();
 
     // FIXME: happens on macosx, don't know why
     if (releases == null)
@@ -237,9 +238,6 @@ private String setButtonOrLink(JButton button, String desc, String label, String
 
   // TODO Make this a method of Theme
   private JTextPane makeNewDescription() {
-    if (getComponentCount() > 0) {
-      remove(0);
-    }
     JTextPane description = new JTextPane();
     description.setInheritsPopupMenu(true);
     Insets margin = description.getMargin();
@@ -265,7 +263,6 @@ private JTextPane makeNewDescription() {
       }
     });
     // description.addKeyListener(new DelegatingKeyListener(parentTable));
-    add(description, 0);
     return description;
   }
 
diff --git a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java
index d20d708530c..dc978be32f3 100644
--- a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java
+++ b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java
@@ -75,6 +75,7 @@ public class ContributedPlatformTableCellJPanel extends JPanel {
   final JPanel buttonsPanel;
   final JPanel inactiveButtonsPanel;
   final JLabel statusLabel;
+  final JTextPane description;
   private final String moreInfoLbl = tr("More Info");
   private final String onlineHelpLbl = tr("Online Help");
 
@@ -117,7 +118,8 @@ public ContributedPlatformTableCellJPanel() {
     versionToInstallChooser
         .setMaximumSize(versionToInstallChooser.getPreferredSize());
 
-    makeNewDescription();
+    description = makeNewDescription();
+    add(description);
 
     buttonsPanel = new JPanel();
     buttonsPanel.setLayout(new BoxLayout(buttonsPanel, BoxLayout.X_AXIS));
@@ -187,8 +189,6 @@ private String setButtonOrLink(JButton button, String desc, String label, String
   void update(JTable parentTable, Object value, boolean hasBuiltInRelease) {
     ContributedPlatformReleases releases = (ContributedPlatformReleases) value;
 
-    JTextPane description = makeNewDescription();
-
     // FIXME: happens on macosx, don't know why
     if (releases == null) {
       return;
@@ -279,9 +279,6 @@ void update(JTable parentTable, Object value, boolean hasBuiltInRelease) {
   }
 
   private JTextPane makeNewDescription() {
-    if (getComponentCount() > 0) {
-      remove(0);
-    }
     JTextPane description = new JTextPane();
     description.setInheritsPopupMenu(true);
     Insets margin = description.getMargin();
@@ -305,7 +302,6 @@ private JTextPane makeNewDescription() {
         Base.openURL(e.getDescription());
       }
     });
-    add(description, 0);
     return description;
   }
 

From af9e859c0866171d2e87ae9e34eec5cb8fa9edff Mon Sep 17 00:00:00 2001
From: Matthijs Kooijman <matthijs@stdin.nl>
Date: Tue, 1 Oct 2019 15:13:04 +0200
Subject: [PATCH 3/3] Set foreground color in library/board manager

Previously, only the background color was changed to white or light
grey. This worked well for the default theme with a black or dark text,
but not for a dark theme with white or light text.

This commit fixes this by also overriding the text color to be black.
Since the colors are set on the JPanel table cell, but the actual text
is rendered by the description JTextPane, the `setForeground` method is
overridden to forward the foreground color to the description pane.

Note that this commit only touches the table cell and description
inside, the dropdowns and buttons have neither background nor foreground
color set (thus these use both colors from the system theme). It might
be more consistent to also override these, but such native UI components
are typically tricky to colorize properly, so best let the system handle
that normally.

An alternative solution would be only use the default colors, which
would actually preserve the dark theme colors in these managers as well
(rather than forcing black-on-white/grey as now). There are default
colors for selected and non-selected table cells that could be used, but
these are different from the current colors. Additionally, the current
odd/even alternating colors are then also no longer available.
---
 .../libraries/ui/ContributedLibraryTableCellEditor.java    | 1 +
 .../libraries/ui/ContributedLibraryTableCellJPanel.java    | 7 +++++++
 .../libraries/ui/ContributedLibraryTableCellRenderer.java  | 1 +
 .../packages/ui/ContributedPlatformTableCellEditor.java    | 1 +
 .../packages/ui/ContributedPlatformTableCellJPanel.java    | 6 ++++++
 .../packages/ui/ContributedPlatformTableCellRenderer.java  | 1 +
 6 files changed, 17 insertions(+)

diff --git a/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellEditor.java b/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellEditor.java
index c5c2e41751c..7c2ecff383f 100644
--- a/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellEditor.java
+++ b/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellEditor.java
@@ -125,6 +125,7 @@ public Component getTableCellEditorComponent(JTable table, Object value,
     editorCell.versionToInstallChooser
         .setVisible(!mayInstalled.isPresent() && notInstalled.size() > 1);
 
+    editorCell.setForeground(Color.BLACK);
     editorCell.setBackground(new Color(218, 227, 227)); // #dae3e3
     return editorCell;
   }
diff --git a/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellJPanel.java b/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellJPanel.java
index cbef1ba3870..46182bd0c5d 100644
--- a/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellJPanel.java
+++ b/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellJPanel.java
@@ -271,4 +271,11 @@ public void setButtonsVisible(boolean enabled) {
     buttonsPanel.setVisible(enabled);
     inactiveButtonsPanel.setVisible(!enabled);
   }
+
+  public void setForeground(Color c) {
+    super.setForeground(c);
+    // The description is not opaque, so copy our foreground color to it.
+    if (description != null)
+      description.setForeground(c);
+  }
 }
diff --git a/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellRenderer.java b/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellRenderer.java
index bc4b3ffd940..d107f90208a 100644
--- a/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellRenderer.java
+++ b/app/src/cc/arduino/contributions/libraries/ui/ContributedLibraryTableCellRenderer.java
@@ -46,6 +46,7 @@ public Component getTableCellRendererComponent(JTable table, Object value,
         value, isSelected);
     cell.setButtonsVisible(false);
 
+    cell.setForeground(Color.BLACK);
     if (row % 2 == 0) {
       cell.setBackground(new Color(236, 241, 241)); // #ecf1f1
     } else {
diff --git a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellEditor.java b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellEditor.java
index acfb58bb3ea..7fe221fa340 100644
--- a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellEditor.java
+++ b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellEditor.java
@@ -130,6 +130,7 @@ public Component getTableCellEditorComponent(JTable table, Object _value,
         .setVisible(installed == null && uninstalledReleases.size() > 1);
 
     cell.update(table, _value, !installedBuiltIn.isEmpty());
+    cell.setForeground(Color.BLACK);
     cell.setBackground(new Color(218, 227, 227)); // #dae3e3
     return cell;
   }
diff --git a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java
index dc978be32f3..100c7b8b12b 100644
--- a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java
+++ b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellJPanel.java
@@ -312,4 +312,10 @@ public void setButtonsVisible(boolean enabled) {
     inactiveButtonsPanel.setVisible(!enabled);
   }
 
+  public void setForeground(Color c) {
+    super.setForeground(c);
+    // The description is not opaque, so copy our foreground color to it.
+    if (description != null)
+      description.setForeground(c);
+  }
 }
diff --git a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellRenderer.java b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellRenderer.java
index 19d484dc23e..b6f6aae015c 100644
--- a/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellRenderer.java
+++ b/app/src/cc/arduino/contributions/packages/ui/ContributedPlatformTableCellRenderer.java
@@ -46,6 +46,7 @@ public Component getTableCellRendererComponent(JTable table, Object value,
     cell.setButtonsVisible(false);
     cell.update(table, value, false);
 
+    cell.setForeground(Color.BLACK);
     if (row % 2 == 0) {
       cell.setBackground(new Color(236, 241, 241)); // #ecf1f1
     } else {