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

Add auto group colour assignment #10521

Merged
merged 18 commits into from
Dec 23, 2023

Conversation

Ziyao-Jin
Copy link
Contributor

@Ziyao-Jin Ziyao-Jin commented Oct 18, 2023

Assign colour with maximal HSV distance when create a new group. Greatest distance is used for top level groups. I have also implemented method for sub level groups using single hue scale, but I am having trouble distinguishing between top level groups and sub level groups while creating a new group now. So, the colour of the sub group in the screenshot does not use the same hue scale as their top level group.

Fixes #7613

This is my first pr, I appreciate feedback from all aspects. If there's anything I did wrong please correct me.

image

Mandatory checks

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@ThiloteE
Copy link
Member

ThiloteE commented Oct 20, 2023

I tried this PR. Can confirm the groups automatically get assigned different group colours. I am more of a user, so i will not comment on coding details.

image

Feedback

  1. For some reason, the first two groups I created were both redish (ww, sdfs). I would have expected them to deviate further.
  2. Distinguishing subgroups from top-level groups, while at the same time conveying a relationship between them, indeed is hard. A certain balance is needed. If technically possible, I would prefer, if at least, sub-groups of the same level have only small colour deviations from each other, as you already tried to implement. As you can see it worked pretty well for the ujujuj sub-sub-groups (greenish yellow), but the last sub-sub-group I added (hmhmh) is reddish again for some reason. I assume it is because I only had added hmhmh after hjjmm was added or something like that.
  3. We have had some users in the past that asked for groups to always use the same colour they had used the last time they created a group. Implementing this feature will prevent them from doing so. I am not against implementing this feature, but it would be nice, if we could find a way to have both worlds.
  4. This feature here implements a change that maybe unnecessarily adds too many colours to groups. Groups now look like a christmas tree. See also https://chartio.com/learn/charts/how-to-choose-colors-data-visualization/#avoid-unnecessary-usage-of-color.

To conclude: I personally would prefer if this feature was optional and activated by user choice.
image

@Ziyao-Jin
Copy link
Contributor Author

WX20231020-211639@2x
My implement will look like this if I could figure out how to distinguish the top level group and subgroup. The brightness will increase based on the level of subgroup.

It is a great idea to make this feature optional and activated by user choice. I will try to add the clickable option.
Thank you for your feedback.

@Siedlerchr
Copy link
Member

You might find the group

Optional<GroupTreeNode> rootGroup = currentDatabase.getMetaData().getGroups();
                
                if (rootGroup.isPresent()) {
                    LIst<Group> groupsWithSameName = rootGroup.get().findChildrenSatisfying(group -> group.getName().equals(name))

and then you get it with any children

@AEgit
Copy link

AEgit commented Oct 22, 2023

I would recommend testing this implementation with a database with thousands of groups and subgroups and a large groups tree depth (e.g., a depth of 20 to 30). I wonder how things will look like in that case.

@Ziyao-Jin
Copy link
Contributor Author

image image

I've made this feature optional and added a button that allows users to choose whether to activate the automatic color assignment. My implementation of single hue color assignment for subgroups was not successful because it involves a lot of refactoring. The current effect will look similar to my initial comment, but the colors will have more deviation. By default, this feature is turned off unless users wish to manually activate the automatic color distribution function.

@Ziyao-Jin Ziyao-Jin marked this pull request as ready for review October 27, 2023 19:42
@koppor koppor mentioned this pull request Oct 28, 2023
6 tasks
@koppor
Copy link
Member

koppor commented Oct 28, 2023

I don't know how this works. Always gray as new color...

image

@Ziyao-Jin
Copy link
Contributor Author

Sorry, I forgot to mention that users need to click the button under the colour picker labeled 'Auto:OFF' to activate this feature. However, I'm not sure if this setting is intuitive.

@Siedlerchr
Copy link
Member

A checkbox would be more intuitive than a button to enable/disable the feature

@Ziyao-Jin
Copy link
Contributor Author

image

Would it be better if it's done this way?

@koppor
Copy link
Member

koppor commented Oct 29, 2023

I would expect it as follows:

New group creation: color automatically set

Existing group edit: Button next to the color with "refresh" circle as icon. On click: New auto color is assigned.

No need for a setting to activate and deactivate

@Ziyao-Jin
Copy link
Contributor Author

Should I provide an option for users to enable this feature themselves in order to adhere to the rule of avoiding unnecessary usage of color?

@koppor
Copy link
Member

koppor commented Oct 29, 2023

Good point with too many colors. I am currently afraid of the WTFs of the checkbox present at group modification. Because checking and unchecking has no effect there. In case there is a help button next to it linking to user documentation for group coloring, it is OK for me. Add a section to the groups help in user-documentstion repo.

@Ziyao-Jin
Copy link
Contributor Author

Ziyao-Jin commented Oct 29, 2023

Currently, the automatic color assignment after clicking the checkbox will only take effect the next time the dialog box is opened. I can change it so that when the user clicks the checkbox, a random color is immediately generated and replaces the current color in the color picker (if clicked again, it will immediately revert to gray). If it's made this way, it would be more intuitive, so would it eliminate the need for a help button? Additionally, do I need to add a refresh button?

@Siedlerchr
Copy link
Member

I can change it so that when the user clicks the checkbox, a random color is immediately generated and replaces the current color in the color picker (if clicked again, it will immediately revert to gray).

Yes this sounds like a good idea

@Ziyao-Jin
Copy link
Contributor Author

image

Now after the user click the checkbox, the color picker will respond immediately.

@koppor
Copy link
Member

koppor commented Nov 2, 2023

if clicked again, it will immediately revert to gray)

I would remove that, because the user can choose another color by his own.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

The code seemed to guess random colors and evaluate them. The issue wanted to work on existing colors.

Attached a new calculation based on "Newton-Cotes formulas"

private static final List<Color> TOP_LEVEL_COLORS = new ArrayList<>();
private static final int TRIES = 10000;

// Get most distinct color
Copy link
Member

Choose a reason for hiding this comment

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

Please remove that comment - it is the same as the method name and thus obsolete

(and this is a good thing, because this is an indication that the method names are well-chosen)

return mostDistinct;
}

// Generate color for top groups
Copy link
Member

Choose a reason for hiding this comment

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

Remove comment

return useRandom ? getDistinctHue() : IconTheme.getDefaultGroupColor();
}

// Generate color for subgroups
Copy link
Member

Choose a reason for hiding this comment

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

Remove comment

private static final int TRIES = 10000;

// Get most distinct color
private static Color getDistinctHue() {
Copy link
Member

Choose a reason for hiding this comment

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

This method doesn't take the existing colors into consideration.

Please adjust the algorithm to use the colors of the existing subgroup (or all the top level groups) and then calculcate THE ONE most distant color.

I would just work with H, not S and V


Some inspiration

(read on later)

import java.awt.Color;
import java.util.ArrayList;
import java.util.List;

public class ColorDistance {

    // Find a Hue value that maximizes the sum of distances
    public static float findFurthestHue(List<Color> colors) {
        float maxDistanceSum = -1;
        float furthestHue = 0;

        // Iterate over the Hue space [0, 1)
        for (int h = 0; h < 360; h++) {
            float candidateHue = h / 360.0f;
            double distanceSum = 0;

            for (Color color : colors) {
                float[] currentHSV = new float[3];
                Color.RGBtoHSB(color.getRed(), color.getGreen(), color.getBlue(), currentHSV);

                float dh = Math.abs(candidateHue - currentHSV[0]);
                dh = Math.min(dh, 1.0f - dh);  // Hue is circular

                distanceSum += dh;
            }

            if (distanceSum > maxDistanceSum) {
                maxDistanceSum = distanceSum;
                furthestHue = candidateHue;
            }
        }

        return furthestHue;
    }

    public static void main(String[] args) {
        // Sample list of colors
        List<Color> colors = new ArrayList<>();
        colors.add(new Color(255, 0, 0));    // Red
        colors.add(new Color(0, 255, 0));    // Green
        colors.add(new Color(0, 0, 255));    // Blue
        colors.add(new Color(255, 255, 0));  // Yellow

        // Find a Hue that is furthest away from all colors in the list
        float furthestHue = findFurthestHue(colors);

        // Output the result
        System.out.println("Furthest Hue: " + furthestHue);
    }
}

Instead of plainly searching, one can use the Newton-Cotes formula.

Applying Newton-Cotes formulas (like the "Trapezoidal Rule" or "Simpson's Rule") used in numerical integration, we can attempt to optimize our search instead of brute-forcing the entire color space.

One approach to find a value that maximizes the sum of distances is to use a heuristic like the median. For circular quantities like Hue, you may want to convert them to a linear space, e.g., by using the sine and cosine values, before finding the average or median.

import java.awt.Color;
import java.util.ArrayList;
import java.util.List;

public class ColorDistance {

    public static float findFurthestHue(List<Color> colors) {
        double sumSin = 0;
        double sumCos = 0;

        for (Color color : colors) {
            float[] hsv = new float[3];
            Color.RGBtoHSB(color.getRed(), color.getGreen(), color.getBlue(), hsv);
            double angle = hsv[0] * 2 * Math.PI;
            sumSin += Math.sin(angle);
            sumCos += Math.cos(angle);
        }

        double meanAngle = Math.atan2(-sumSin, -sumCos) + Math.PI;
        float furthestHue = (float) (meanAngle / (2 * Math.PI));

        return furthestHue;
    }

    public static void main(String[] args) {
        // Sample list of colors
        List<Color> colors = new ArrayList<>();
        colors.add(new Color(255, 0, 0));    // Red
        colors.add(new Color(0, 255, 0));    // Green
        colors.add(new Color(0, 0, 255));    // Blue
        colors.add(new Color(255, 255, 0));  // Yellow

        // Find a Hue that is furthest away from all colors in the list
        float furthestHue = findFurthestHue(colors);

        // Output the result
        System.out.println("Furthest Hue: " + furthestHue);
    }
}

@koppor
Copy link
Member

koppor commented Dec 18, 2023

DevCall discusssions:

  • total number of groups: 10? 1000?
  • How to distinguish elements in the same level?
  • Should a color be globally unique?
  • Recoloring of everything to be consistent? --> Follow-up PR
  • Aim of colors: Distinction between elements
  • Preferences: colorful groups, shaded colored groups; discussion: not same colors for same level
  • horizontal (same level) vs. vertical (sub level) same colors?

Decisions:

  • colorful groups
  • right click on group: recolor all sub groups to same color

We will then check the feedback

@koppor
Copy link
Member

koppor commented Dec 18, 2023

When pressing "OK" the group gets assigned a color. But a user may want to have NO color.

--> We need a checkbox for having a color. If the checkbox is disabled, JabRef removes the color at "OK". Can only be undone if the dialog is NOT closed.

@koppor
Copy link
Member

koppor commented Dec 22, 2023

First shot for calculated auto coloring is there.

Screenshot:

image

The first color of the subgroup is the color of the parent group a bit darker. Not sure if that is a good idea, but I did not want to start totally random at a sub group

Code smell: Auto coloring is saved in a global variable - and not in the preference

I needed some @Nullable annotations and used JSpecify. -- Refs #10240 😅.

koppor
koppor previously approved these changes Dec 22, 2023
Co-authored-by: Carl Christian Snethlage <[email protected]>
@koppor koppor added this pull request to the merge queue Dec 23, 2023
Merged via the queue into JabRef:main with commit 31467be Dec 23, 2023
18 of 19 checks passed
@Ziyao-Jin Ziyao-Jin deleted the 7613-assign-color-automatically-to-group branch December 27, 2023 02:49
@koppor koppor mentioned this pull request Mar 17, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assign color automatically to group
5 participants