Colored Bookmarks (#1866)

wladimirleite commented 12 months ago

I guess this is ready for review. @lfcnassif, I know you have a lot of things to deal with in the next weeks, this is low priority. Anyway, when you get some time, take a quick look, just to see if there is anything (in the visual changes) that need to be improved or redesigned. These GUI changes are very personal... As I mentioned in the issue description, I tried to visually highlight the bookmarks, as they are something added (automatically or by an analyst) to the item (rather than a regular property), with a visual design used by common programs.

lfcnassif commented 12 months ago

Thank you @tc-wleite!

lfcnassif commented 10 months ago

Hi @patrickdalla, could you help me and review and test this next week?

patrickdalla commented 10 months ago

sure

Em sex., 20 de out. de 2023 12:15, Luis Filipe Nassif < @.***> escreveu:

Hi @patrickdalla https://github.com/patrickdalla, could you help me and review and test this next week?

— Reply to this email directly, view it on GitHub https://github.com/sepinf-inc/IPED/pull/1887#issuecomment-1773024628, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG247S3J5ZASNK3VTDZOSV3YAKPSJAVCNFSM6AAAAAA435X722VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZTGAZDINRSHA . You are receiving this because you were mentioned.Message ID: @.***>

patrickdalla commented 10 months ago

Done tests and results (Single case):

Bookmark created: resulted in colored bookmark label background on each item, and little square icon with same color on bookmark tree.
Bookmark color changed: bookmark items label background colour changed to the selected colour, as the little square icon color.
Bookmarks saved, deleted and later reloaded: All original selected colors were correctly restored.
Bookmark changed to an already attributed color to other bookmark: Both corresponding bookmark items label background were redenred with same color.
Galery View: Square icon colored with the corresponding bookmark selected color is rendered before item name at the top of the thumb. If the item is part of more than one bookmark, the square icon is striped vertically to render each corresponding color.
Bookmark color change with Galery View opened: Color change is reflected in each item bookmark square item.
HTML Report: Bookmark colors aren't shown anywhere in resulting html report.

@tc-wleite No exception was thrown and everything seems to result as expected. I saw that no implementation was done to reflect bookmark colors on HTML report, so this is not really an error, just an observation if you want to complete this.

patrickdalla commented 10 months ago

For multicase: It does not open the bookmark manager panel. Nullpointer is thrown at line 328 of Bookmarks.java. As the bookmark of one case is not found in other cases, -1 is passed as bookmark ID, leading in the nullpointer exception.

wladimirleite commented 10 months ago

For multicase: It does not open the bookmark manager panel. Nullpointer is thrown at line 328 of Bookmarks.java. As the bookmark of one case is not found in other cases, -1 is passed as bookmark ID, leading in the nullpointer exception.

Thanks @patrickdalla! I am looking into this error.

wladimirleite commented 10 months ago

For multicase: It does not open the bookmark manager panel. Nullpointer is thrown at line 328 of Bookmarks.java. As the bookmark of one case is not found in other cases, -1 is passed as bookmark ID, leading in the nullpointer exception.

If bookmarkId is -1, the line 328 will just return null, not throw a NPE, right? line 328 of Bookmarks class: return bookmarkColors.get(bookmarkId);

I could only reproduce this exception mixing cases that were processed with an older version with cases processed with the new version, when opening the multicase. In this scenario, a NPE is thrown because bookmarkColors map is null. I guess this won't be a common usage, but I will try to change the code to handle this situation.

@patrickdalla, can you confirm if you were using only cases processed with this branch (or not) when opened the multicase?

wladimirleite commented 10 months ago

@patrickdalla, pushed a commit that avoid the exception in line 328, but as I said, I could only reproduced it mixing cases, so maybe it won't solve the issue in your case. Let me know if you still get the error.

patrickdalla commented 10 months ago

Yes. You are right. I tried to reproduce my imagined cause of the error, but I couldn't, so, the error seems to be caused by what you have corrected. In fact I did not processed any new case.

patrickdalla commented 10 months ago

As bookmark is a feature of IpedApp, I think it is not necessary to reprocess the case. Am I wrong? Maybe only remove any saved bookmark files.

patrickdalla commented 10 months ago

But it seems this same cause affects other parts of the code, as in BookmarkColorsUtil.getForeground:

Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException at iped.app.ui.bookmarks.BookmarkColorsUtil.getForeground(BookmarkColorsUtil.java:70) at iped.app.ui.bookmarks.BookmarkCellRenderer.setBookmark(BookmarkCellRenderer.java:57)

wladimirleite commented 10 months ago

It should now work fine opening multicases processed without this feature. I tested before opening a single case processed with a previous version, but not multicases.

Anyway, it would be nice to test processing with this feature, as there was a small change regarding automatically created bookmarks.

wladimirleite commented 10 months ago

But it seems this same cause affects other parts of the code, as in BookmarkColorsUtil.getForeground:

Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException at iped.app.ui.bookmarks.BookmarkColorsUtil.getForeground(BookmarkColorsUtil.java:70) at iped.app.ui.bookmarks.BookmarkCellRenderer.setBookmark(BookmarkCellRenderer.java:57)

I am looking into this one.

wladimirleite commented 10 months ago

But it seems this same cause affects other parts of the code, as in BookmarkColorsUtil.getForeground: Exception in thread "AWT-EventQueue-0" java.lang.NullPointerException at iped.app.ui.bookmarks.BookmarkColorsUtil.getForeground(BookmarkColorsUtil.java:70) at iped.app.ui.bookmarks.BookmarkCellRenderer.setBookmark(BookmarkCellRenderer.java:57)

I am looking into this one.

Pushed another commit to avoid this exception.

patrickdalla commented 10 months ago

An observation: If I load 2 cases in a multicase, both have one bookmark with the same name but different colors, on color is chosen, maybe the first loaded color. The chosen color is applied in the case where there were a different color, if I open it alone later, even if I make no modifications in bookmarks at all. So, if someone opens a case analysed by someone else with some other cases, it overrides this color information.

wladimirleite commented 10 months ago

An observation: If I load 2 cases in a multicase, both have one bookmark with the same name but different colors, on color is chosen, maybe the first loaded color. The chosen color is applied in the case where there were a different color, if I open it alone later, even if I make no modifications in bookmarks at all. So, if someone opens a case analysed by someone else with some other cases, it overrides this color information.

This same behavior already happens with comments and shortcut keys, right?

patrickdalla commented 10 months ago

Yes. Maybe the solution could be let to a specific different issue, as it will be more complete.

patrickdalla commented 10 months ago

At some point in multicase (after changing data in original single cases and coming back to multicase) this strange behaviou start happening: The color change of single bookmark affect others:

https://github.com/sepinf-inc/IPED/assets/28692427/d25efd7d-f68c-4734-a724-0d8a3e1d47ae

patrickdalla commented 10 months ago

The code is adding in bookmarkColors object, an TreeMap, all the -1 values, returned by not found bookmark name on case that do not have these bookmarks. Later, when getBookmarkColor is called, this Color associated with -1 value (bookmark name not found) are found.

patrickdalla commented 10 months ago

It passed the tests for multicase analysis. @tc-wleite What would be the "small change regarding automatically created bookmarks"? I want to prepare a case to test it.

patrickdalla commented 10 months ago

An observation: If I load 2 cases in a multicase, both have one bookmark with the same name but different colors, on color is chosen, maybe the first loaded color. The chosen color is applied in the case where there were a different color, if I open it alone later, even if I make no modifications in bookmarks at all. So, if someone opens a case analysed by someone else with some other cases, it overrides this color information.

@lfcnassif about this overwriting problem? Should we create a new separate issue to solve it, as it is not specific to colored bookmark? Or you think this is "not a problem"?

wladimirleite commented 10 months ago

It passed the tests for multicase analysis. @tc-wleite What would be the "small change regarding automatically created bookmarks"? I want to prepare a case to test it.

It is possible to define colors for automatically created bookmarks. I set colors for some of them (e.g. shared through e-Mule or WhatsApp).

lfcnassif commented 10 months ago

@lfcnassif about this overwriting problem? Should we create a new separate issue to solve it, as it is not specific to colored bookmark? Or you think this is "not a problem"?

If it is mixing comments and shortcut keys as @tc-wleite said, yes, we should open a separate ticket to fix it and report the fix in the release notes. Fixing it here is possible to avoid merge conflicts, but in this case it will be released just together with the colored bookmarks feature, which should be on 4.2.0.

wladimirleite commented 10 months ago

If it is mixing comments and shortcut keys as @tc-wleite said, yes, we should open a separate ticket to fix it and report the fix in the release notes. Fixing it here is possible to avoid merge conflicts, but in this case it will be released just together with the colored bookmarks feature, which should be on 4.2.0.

And what would be the solution? Case A has Bookmark X with comment XA Case B has Bookmark X with comment XB What would it be displayed (and saved) if A and B are opened as a multicase?

It doesn't seem like a common situation. Unless there is a simple solution, I am not sure if adding something complex to handle this would be a good idea.

lfcnassif commented 10 months ago

And what would be the solution? Case A has Bookmark X with comment XA Case B has Bookmark X with comment XB What would it be displayed (and saved) if A and B are opened as a multicase?

It doesn't seem like a common situation. Unless there is a simple solution, I am not sure if adding something complex to handle this would be a good idea.

Bookmarks class is based on integer IDs and could allow 2 bookmarks with the same name in theory, but I protected against it in UI code to avoid such a confusing situation. Unfortunately I didn't anticipate this situation in multicases and the MultiBookmarks API is based on bookmark names (Strings), I think it would need to be refactored. Since it is just a proxy class that redirects to the right Bookmarks class instance of the right case, I think it could be fixed, but would be a backwards incompatible API change... UI code would also need to be updated to allow 2 Bookmarks with the same name with all related operations (create, delete, add, remove, update comment, etc...).

wladimirleite commented 10 months ago

Bookmarks class is based on integer IDs and could allow 2 bookmarks with the same name in theory, but I protected against it in UI code to avoid such a confusing situation. Unfortunately I didn't anticipate this situation in multicases and the MultiBookmarks API is based on bookmark names (Strings), I think it would need to be refactored. Since it is just a proxy class that redirects to the right Bookmarks class instance of the right case, I think it could be fixed, but would be a backwards incompatible API change... UI code would also need to be updated to allow 2 Bookmarks with the same name with all related operations (create, delete, add, remove, update comment, etc...).

But wouldn't it be worse for the user? For example, opening several cases with an automatically created bookmark in each (e.g. Probably Shared By WhatsApp), would it show as several bookmarks in the multicase?

lfcnassif commented 10 months ago

But wouldn't it be worse for the user? For example, opening several cases with an automatically created bookmark in each (e.g. Probably Shared By WhatsApp), would it show as several bookmarks in the multicase?

Yes... Maybe prefix bookmarks from different cases with a number prefix would avoid confusion?

However, if user creates a new bookmark in a multicase with items from different cases, today that new bookmark is created in each single case and appears in multicase UI as a single Bookmark, which seems correct. Using a prefix would display several bookmarks in this situation, so I'm not sure what would be the solution...

wladimirleite commented 10 months ago

Yes... Maybe prefix bookmarks from different cases with a number prefix would avoid confusion?

Not sure... Probably the user would prefer to have a single bookmark, as it is today.

patrickdalla commented 10 months ago

Could we create a separate bookmark.iped file for the multicase? When opening a multicase, if bookmark conflicts are identified a merge warning could be shown. And any modification would apply to this separate bookmark.iped for the multicase, keeping the original single case bookmark.iped.

Em ter., 24 de out. de 2023 17:15, Wladimir Leite @.***> escreveu:

Yes... Maybe prefix bookmarks from different cases with a number prefix would avoid confusion?

Not sure... Probably the user would prefer to have a single bookmark, as it is today.

— Reply to this email directly, view it on GitHub https://github.com/sepinf-inc/IPED/pull/1887#issuecomment-1778051642, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG247SYVPP5ORLQ6OFVUNALYBAVXXAVCNFSM6AAAAAA435X722VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZYGA2TCNRUGI . You are receiving this because you were mentioned.Message ID: @.***>

patrickdalla commented 10 months ago

I've opened the issue #1946 to discuss there.

patrickdalla commented 10 months ago

The processing of some cases finished and the bookmarks created with their expected color. So I think this test has passed also. So, my review has finished, and i think it is ready for merge @tc-wleite @lfcnassif .

lfcnassif commented 10 months ago

Thanks @tc-wleite for this PR and thanks @patrickdalla for reviewing!

sepinf-inc / IPED

Colored Bookmarks (#1866) #1887