cdown / clipmenu

Clipboard management using dmenu
MIT License
1.1k stars 90 forks source link

Deduplicate entries #224

Open N-R-K opened 2 months ago

N-R-K commented 2 months ago

Currently, duplicate snips get multiple entries clogging up the list of clips. This is especially problematic when you're using a small number of max clips (e.g 32). It'd be nice to avoid duplicate entries from being registered.

cdown commented 2 months ago

If they are contiguous indexes they should not duplicate, that's handled by is_possible_partial. Could you give an example?

N-R-K commented 2 months ago

Could you give an example?

Sure, here's a test case:

$ xclip -sel c <<< "dup"
$ xclip -sel c <<< "clone"
$ xclip -sel c <<< "dup"
$ xclip -sel c <<< "clone"

Now if I open clipmenu, there's 2 "dup" and 2 "clone" entries in there.

image

cdown commented 2 months ago

Yeah, these are not contiguous entries. This is actually intentional: one of the pieces of feedback from "old" clipmenu was that deduplication broke the sense of time of copying. In fact we have code specifically to preserve this behaviour.

N-R-K commented 2 months ago

deduplication broke the sense of time of copying

I'm not sure what this means, but I'm assuming it means the following:

  1. You have a snip "A" at index 10
  2. You copy "A" again
  3. "A" remains at index 10 instead of becoming index 0

Is that correct? If yes, then it does not seem like people want duplicate entries but rather they want new duplicate's "timestamp" to be bumped/renewed.

I haven't looked into src/store.c yet, but I can start digging to see how to make that work if I understood the problem correctly.

cdown commented 2 months ago

It means you have entries:

A
B
C
D
E

You then copy B again. In your suggestion, the new entries are:

A
C
D
E
B

But the desired behaviour is actually:

A
B
C
D
E
B

Obviously one can make an argument in favour of either approach, but we used to do deduplication across the entire line cache and people would complain that their clips "disappeared", when in fact they just moved around after being copied again.

N-R-K commented 2 months ago

but we used to do deduplication across the entire line cache and people would complain that their clips "disappeared", when in fact they just moved around after being copied again.

Ah I see, so they do want duplicated entries.

Obviously one can make an argument in favour of either approach

Seems like there's no "objective" right behavior here, so would a config var to do de-duplication be acceptable then? It would serve both cases, user gets to pick the behavior he prefers.

cdown commented 2 months ago

A config option is fine as long as it doesn't blow up the code complexity/size too much. Thanks!

N-R-K commented 2 months ago

I've poked a bit into this. The clip_store thing seems pretty delicate and I haven't looked through it fully, but something like the following seems to be working, in pseudo-code:

// in cs_content_add
if (dupe && do_dedup) {
    _drop_ ... = cs_ref(..);
    int dup_index = find_hash(cs->snips);
    move_to_end(cs->snips, dup_index);
    return 1;
}

Is this okay? Or will it cause some problem.