feature:extract color and search

[andjar]

I got some help to implement colorthief (see #1064 ); now, a palette is made for each photo, and presented under info, and one can search by clicking on a color. See lower right corner in the screenshot [painting: Livets dans by Edvard Munch]:

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[ildyria]

Okay, first of all, I love it. :)

However I think it is better to wait a bit for this to be merged. What I mean is that @nagmat84 is currently undergoing a significant refactoring of the code, and merging after his changes makes more sense.

Now, I'm wondering whether it is better to have the color relationship as you implemented. Or have directly a table color palette which associate the 5 colors in one go...

@nagmat84 what would be your take on this?

Implementation wise, I would make use of a trait for the color palette extraction (idea of Single Responsibility Principle), and then have the Photo::create() call the function where you had your code originally. This also gives a bit more modularity to the system.

Now, on the missing parts to add:

1. A CLI command to parse already existing pictures to fill up their color palette.
2. Double check what happens when you delete a picture...
3. What happens when you duplicate a picture ?

By using a color palette instead of a set of color, you allow a has_one photo -> palette and has_many palette -> photo without having to duplicate the colors.

4. Add a function call "Extract color palette" ? :)

[nagmat84]

I love this feature, too. :heart_eyes:

Now, I'm wondering whether it is better to have the color relationship as you implemented. Or have directly a table color palette which associate the 5 colors in one go...

I don't get your point. There is a table colors, isn't it? And what 5 colors are you referring to?

Implementation wise, I would make use of a trait for the color palette extraction (idea of Single Responsibility Principle), and then have the Photo::create() call the function where you had your code originally. This also gives a bit more modularity to the system.

I disagee . Using a trait here is not the right approach. (In particular, a trait is not the solution to the single responsibility principle.) If you wanted to decouple the code from the method Create::do and move out the code to an independent class, then you would have to create some kind of ColorPaletteExtractionService (or something like that) in the same spirit as we already have a MetadataExtractor, get an instance of the ColorPaletteExtractionService where you need it and call it. But this seems a little bit over the edge for these extra 10 lines of code.

Anyway, I am really undecided, if the code may remain where it is right now or if I would prefer having a de-coupled ColorPaletteExtractionService. But please don't use a trait!

[ildyria]

I love this feature, too.

Now, I'm wondering whether it is better to have the color relationship as you implemented. Or have directly a table color palette which associate the 5 colors in one go...

I don't get your point. There is a table colors, isn't it? And what 5 colors are you referring to?

When you query a Color palette, you will get 5 colors in return which you will need to save.

In the current set up, you have 1 photo = 5 new color rows.

I am just wondering about the size explosion of such table and whether a different implementation would change this. I am currently thinking of:

1. ~~Having a table color_palette with basically 16 columns which incorporate the attributes: id, (rgb)1, (rgb)2, (rgb)3, (rgb)4, (rgb)5 But this would complexity the search.~~ But this is a bad idea.
2. Adding an intermediate table "color_palette" which would link the picture to a set of 5 colors: palette_id, color_1_id, color_2_id, color_3_id, color_4_id, color_5_id, where 1 is the main color.

As I see it, this second approach as multiple benefits, it remove the need to define a boolean for primary color within the color table (as it would be color_1_id). Futhermore, you can do eager loading on the colors. And finally, it solves the duplication problem: you do not need to duplicate the colors when you duplicate the picture and you move to a has_one relationship color->palette instead of a has_many.

In the end you would get: Photo -> palette -> colors.

I would be leaning towards such step.

Implementation wise, I would make use of a trait for the color palette extraction (idea of Single Responsibility Principle), and then have the Photo::create() call the function where you had your code originally. This also gives a bit more modularity to the system.

I disagee . Using a trait here is not the right approach. (In particular, a trait is not the solution to the single responsibility principle.) If you wanted to decouple the code from the method Create::do and move out the code to an independent class, then you would have to create some kind of ColorPaletteExtractionService (or something like that) in the same spirit as we already have a MetadataExtractor, get an instance of the ColorPaletteExtractionService where you need it and call it. But this seems a little bit over the edge for these extra 10 lines of code.

My mistake, though I still think this codes need to be extracted, so it can be reused in CLI. This code will be duplicated otherwise. Furthermore it makes it less likely to conflict with your refactoring and easier to integrate with your strategies. :)

[nagmat84]

When you query a Color palette, you will get 5 colors in return which you will need to save.

Are we sure that this will ever be the case? But it's fine for me to have that as an assumption.

In the current set up, you have 1 photo = 5 new color rows. I am just wondering about the size explosion of such table and whether a different implementation would change this.

I would not be worried about that. We also have 1 photo with 7 different size variants. I am confident that space should not be a problem.

I am currently thinking of:

1. ~Having a table color_palette with basically 16 columns which incorporate the attributes: id, (rgb)1, (rgb)2, (rgb)3, (rgb)4, (rgb)5 But this would complexity the search.~ But this is a bad idea.

I agree. Bad idea.

2. Adding an intermediate table "color_palette" which would link the picture to a set of 5 colors: palette_id, color_1_id, color_2_id, color_3_id, color_4_id, color_5_id, where 1 is the main color.

As I see it, this second approach as multiple benefits, it remove the need to define a boolean for primary color within the color table (as it would be color_1_id).

Fine for me.

Furthermore, you can do eager loading on the colors.

You could also do that with the current approach. That's no different.

And finally, it solves the duplication problem: you do not need to duplicate the colors when you duplicate the picture and you move to a has_one relationship color->palette instead of a has_many.

I believe this is a bad idea, because it does not reflect the true semantics and feels really awkward.

It appears to me as if you wanted to solve a different problem here but with the wrong approach. I agree that duplication of photos could be improved. The specific problem which you mention in this context regarding the colors is actually only one incarnation of a more general problem. We have the same problem with the size variants, the symbolic links and the access rights. Also we have the problem that we only must erase the binary photo files when the last database entry is erased.

Actually, the right approach to solve all these problems together would be to have an m:n relation between albums and photos using a album_photo_associations table. That way duplication of a photo would be a breeze and only require an additional small entry in the table album_photo_associations without duplication of any photo data (not only color data). I already have proposed that change some time back, but you did not like it. Anyway we should not discuss this problem here but take it to Gitter (if you want), because it is beyond the scope of this PR.

In the end you would get: Photo -> palette -> colors. I would be leaning towards such step.

Understood, but surprisingly, see above. This actually introduces a m:n relationship but through the back door :-) and (IMHO) at the wrong place.

Either I would leave the current approach as it is (with the is_main flag) or use the following alternative approach. This alternative also removes the need for the is_primary flag and rules out any inconsistencies (which could happen, if the is_primary flag is not unique among all colors of the same photo):

In pseudo UML

+-----------------------+                       +----------------+
|          Photo        |                       |      Color     |
+-----------------------+                       +----------------+
| id:           bigint  | (1)--<--owned-by--(1) | id:    bigint  |
| primaryColor: Color   | (1)---has-->---(0..1) | photo: Photo   |
| colors:       Color[] | (1)---has-->---(0..*) | r:     int     |
+-----------------------+                       | g:     int     |
                                                | b:     int     |
                                                +----------------+

In (Postge)SQL

CREATE TABLE photos(
  id                SERIAL PRIMARY KEY,
  primary_color_id  INT NULLABLE,
  FOREIGN KEY(primary_color_id) REFERENCES colors(id)
);

CREATE TABLE colors(
  id        SERIAL PRIMARY KEY,
  photo_id  INT NOT NULLABLE,
  r         SMALLINT NOT NULLABLE,
  g         SMALLINT NOT NULLABLE,
  b         SMALLINT NOT NULLABLE,
  FOREIGN KEY(photo_id) REFERENCES photos(id)
);

In PHP with Eloquent

/**
 * @property Color      primary_color
 * @property Collection colors
 */
class Photo extends Model {
  public function primary_color(): HasOne {
    return $this->hasOne(Color::class, 'primary_color_id', 'id');
  }
  
  public function colors(): HasMany {
    return $this->hasMany(Color::class);
  }
}
  
/**
 * @property Photo photo
 */
class Color extends Model {
  public function photo(): BelongsTo {
    return $this->belongsTo(Photo::class);
  }
}

If you wanted to decouple the code from the method Create::do and move out the code to an independent class, then you would have to create some kind of ColorPaletteExtractionService (or something like that) in the same spirit as we already have a MetadataExtractor, get an instance of the ColorPaletteExtractionService where you need it and call it.

I still think this codes need to be extracted, so it can be reused in CLI. This code will be duplicated otherwise. Furthermore it makes it less likely to conflict with your refactoring and easier to integrate with your strategies. :)

OK, then lets move the code to a service class. However, I wonder if it should be its own service (something like ColorPaletteExtractionService) or just be integrated into the metadata extractor. In the end, the histogram is also some kind of metadata and I assume that we want to extract the histogram every time we also extract the other metadata.

[scottlimmer]

What is the status of this PR? I'd like to implement #1064 but I don't want to step any any toes.

[andjar]

Hi, I'm sorry that I didn't follow up the PR; I don't have capacity to look more into this, so please feel free to do what you like @scottlimmer. The code in this PR is working perfectly, but as you can see, there are still some issue. Feel free to copy what you want:)

[nagmat84]

The status is the same as before

1. the requested changes should be implemented, and
2. it would be more easy to merge the other PRs (with the massive refactoring of the architecture) first and then rebase this PR on the new architecture

As the original submitter of this PR has never replied to the requested changes, I actually would be willing to take the effort to apply the requested changes myself and also do the re-basing. I really like the feature and want to see it being integrated. However, first the other PRs must be merged.

[ildyria]

As the original submitter of this PR has never replied to the requested changes, I actually would be willing to take the effort to apply the requested changes myself and also do the re-basing. I really like the feature and want to see it being integrated. However, first the other PRs must be merged.

I have exactly the same opinion. :)

With regard to this:

+-----------------------+                       +----------------+
|          Photo        |                       |      Color     |
+-----------------------+                       +----------------+
| id:           bigint  | (1)--<--owned-by--(1) | id:    bigint  |
| primaryColor: Color   | (1)---has-->---(0..1) | photo: Photo   |
| colors:       Color[] | (1)---has-->---(0..*) | r:     int     |
+-----------------------+                       | g:     int     |
                                                | b:     int     |
                                                +----------------+

I would remove the photo attribute and just make it a m:n relationship. :) Using a pivot table may be necessary though. :|

[scottlimmer]

@ildyria @nagmat84 Sounds like I'll leave this to you.

Are all open issues impacted by pending refactoring PRs?

[nagmat84]

Are all open issues impacted by pending refactoring PRs?

It depends. Minor bug fixes and security issues will be integrated of course. Also, if a PR does not conflict too much with the refactoring, it will likely be integrated.

@scottlimmer: Do you have a specific issue in mind?

[qwerty287]

Due to various refactorings and changes, this will need a complete rework (rewrite from scratch), so I'm going to close it.