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

Make import merge the tag lists by default (when not overwriting) #2023

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

t-w
Copy link

@t-w t-w commented Oct 3, 2023

Implements feature from: #2022

@nodiscc
Copy link
Member

nodiscc commented Oct 3, 2023

Hi,
thanks for the patch. There are a few PHPCS warnings (https://github.com/shaarli/Shaarli/pull/2023/checks), can you please address these?

Also the UI for the overwrite/don't overwrite setting currently looks like this:

image

Maybe it should be updated to clarify what each option does? Currently if the checkbox is unchecked, I would expect existing shaares to not be modified, but with this patch, they will (if the tags are different across duplicates). Maybe a documentation update would be sufficient.

This also removes the possibility of actually not modifying existing shaares. I'm fine with keeping it simple, and deciding on a single "good" behavior, but I'm not sure what is considered "good" by most users. I personally never use this feature, except to import an older backup on a fresh Shaarli installation, so I never run into duplicates. What is the problem you were having with the current behavior (skip shaares with duplicate URLs completely)?

@nodiscc nodiscc removed their request for review October 3, 2023 13:20
@t-w
Copy link
Author

t-w commented Nov 11, 2023

I have a large set (thousands) of bookmarks that I was storing in Google Bookmarks (which was the only reasonable way for me, not requiring any crappy addons, could be used from any device/browser, easily searched etc.). I have them exported and put into Firefox - but this import made a very funny thing - the tags became folders and bookmarks are duplicated for each tag they had... It became a set of more than 40000 bookmarks. There is no way I can order this in any reasonable way (yeah, I can eventually write some tool myself to clean this up... but no time for that so far).
Shaarli seems like a perfect replacement of Google Bookmarks for me - but it must allow merging tag lists for the existing bookmarks, as I will need to synchronize it with Firefox (and so it from time to time, and the last thing I want is losing tags...). Hence the feature request / patch...

As I wrote in #2022, the UI could be extended to include all 3 options, but for me the behaviour with this patch is exactly what I would expect:

  • when you set overwrite, the tag list gets replaced with the new one
  • when it is not overwriting, the tag lists are merged (I do not know how about others, but I was surprised not all the tags were added, I was expecting the opposite...)

Anyway, probably the best would be to add a selection list/box to choose one of the three ways of dealing with tags for existing bookmarks (overwriting or not may concern also description/link title, if I recall well, so such option should probably have description that overwriting/merging/preserving is for tags only).
Then, it would be just conditional executing of the piece of code I added.

The patch is a proposal/starting point for adding the feature. It can surely be improved as needed - but as it is, it is already working well, in my opinion not spoiling anything (ie. it does not introduce any potential data loss, on the contrary!).

IMHO, the UI can be improved later (if needed).

But as you prefer, of course.

(What I care is that the feature is there, as I really need it.)

@nodiscc nodiscc self-requested a review November 15, 2023 02:40
@nodiscc nodiscc added this to the 0.13.0 milestone Nov 15, 2023
@nodiscc
Copy link
Member

nodiscc commented Nov 17, 2023

Thanks a lot for the explanation.

Disregard the failing tests as they are caused by #2029

I have them exported and put into Firefox - but this import made a very funny thing - the tags became folders and bookmarks are duplicated for each tag they had... It became a set of more than 40000 bookmarks. There is no way I can order this in any reasonable way (yeah, I can eventually write some tool myself to clean this up... but no time for that so far).

That's a problem indeed... I've had good results with https://addons.mozilla.org/en-US/firefox/addon/bookmark-dupes/ in the past to get rid of duplicate bookmarks, but I don't think it would help with the merging part.

overwriting or not may concern also description/link title, if I recall well, so such option should probably have description that overwriting/merging/preserving is for tags only

Yes, so we have the possible following behaviors, depending on what the user wants:

  • Do not touch existing bookmarks at all (current behavior if checkbox unchecked)
  • Merge tags if duplicates are found (but do not touch description/title/other data - new behavior with your patch if checkbox unchecked. Useful for importing from Google Bookmarks)
  • Overwrite existing bookmarks if duplicates are found (including tags/description/title/other data - current behavior with checkbox checked)

I think there is value in keeping the possibility of not touching existing bookmarks at all.

The imported data may be "unclean" - For example my existing Shaarli bookmarks are set A, my browser bookmarks are set B, which contains set A plus bookmarks that I only have in Firefox but are missing from Shaarli. In the future, I might want to import bookmarks from my browser, which are missing from Shaarli, but this would add Firefox bookmarks folder names as tags to all my existing Shaarli bookmarks.

IMHO, the UI can be improved later (if needed).

True.

What I suggest is adding a simple configuration setting general.merge_tags_on_import (we can do without the corresponding UI toggle), that is false by default, and when set to true, triggers the tags merging mechanism implemented in your patch.

@nodiscc nodiscc removed their request for review November 18, 2023 00:45
@nodiscc nodiscc modified the milestones: 0.13.0, 0.14.0 Nov 22, 2023
@nodiscc nodiscc modified the milestones: 0.14.0, backlog to the future Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants