Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Imported bookmarks should set creationTime field (not lastAccessedTime) #5183

Closed
srirambv opened this issue Oct 27, 2016 · 10 comments · Fixed by #5349 or #5453
Closed

Imported bookmarks should set creationTime field (not lastAccessedTime) #5183

srirambv opened this issue Oct 27, 2016 · 10 comments · Fixed by #5349 or #5453

Comments

@srirambv
Copy link
Collaborator

Did you search for similar issues before submitting this one?
No

Describe the issue you encountered:
Deleting an imported bookmark doesn't delete the history entry. This comes up as a history suggestion

Expected behavior:
Deleting an imported bookmark should delete its entry from history as well

  • Platform (Win7, 8, 10? macOS? Linux distro?):
    All

  • Brave Version:
    0.12..7 RC1

  • Steps to reproduce:

    1. Import bookmarks from a file or another browser
    2. Delete the imported bookmark
    3. Type the site name in URL, the deleted bookmark shows up as a history suggestion
  • Screenshot if needed:
    history

  • Any related issues:

    cc: @bsclifton Setting milestone as 0.12.8

@bbondy
Copy link
Member

bbondy commented Oct 28, 2016

I don't think this is right, I think deleting a bookmark should be independent of if it shows up in history, if the last access time was set from an import, it should remain there.

@bbondy
Copy link
Member

bbondy commented Oct 28, 2016

Going to close as wontfix, but lmk if you strongly disagree.

@bbondy bbondy removed this from the 0.12.8dev milestone Oct 28, 2016
@bbondy bbondy closed this as completed Oct 28, 2016
@bsclifton
Copy link
Member

bsclifton commented Oct 30, 2016

@bbondy I know someone had opened a similar one and I felt the same way (can't find that ticket...) This one seems to be different because it's related to import (not just deleting a bookmark). It's entirely possible that the user never visited the sites they have imported

Removing a bookmark only removes the tags (not the entry- basically turning it into a history entry)... The real issue here might be that entries in sites should not be considered history items unless there is an associated timestamp. If that's the case, it might be better to reword this ticket as "don't consider entries history unless timestamp is associated". siteUtils already has an "isHistoryEntry" method which checks this

@bbondy
Copy link
Member

bbondy commented Oct 31, 2016

I think if last access date is filled out from a history import then it should be considered a history entry as well. I think if it was never visited last access date will be 0, but I can definitely be wrong on that's how it works (I think it should work that way though). CC @darkdh for his thoughts.

@darkdh
Copy link
Member

darkdh commented Oct 31, 2016

Imported History and Bookmark all have lastAccessedTime info, I think it's my bad to use bookmark creation time as lastAccessedTime when importing bookmarks.
I can definitely to make it 0, what do you think, @bbondy?

@bbondy
Copy link
Member

bbondy commented Nov 1, 2016

is there a lastAccessTime and a creation time you can use? If only creation time is avail maybe set a new field for that and it will be unused for now.

@darkdh
Copy link
Member

darkdh commented Nov 1, 2016

we don't have lastAccessTime when importing bookmarks but I can add a creationTime field for later use

@darkdh darkdh reopened this Nov 1, 2016
@darkdh darkdh removed the wontfix label Nov 1, 2016
@darkdh darkdh self-assigned this Nov 1, 2016
@darkdh darkdh added this to the 0.12.9dev milestone Nov 1, 2016
@bsclifton bsclifton changed the title Deleting an imported bookmark should delete its entry from history as well Imported bookmarks should set creationTime field (not lastAccessedTime) Nov 1, 2016
@bsclifton
Copy link
Member

Updated the title to reflect the current status. Thanks, @darkdh 😄

darkdh added a commit to darkdh/browser-laptop that referenced this issue Nov 2, 2016
2. Add creationTime field

fix brave#5183

Auditors: @bsclifton, @bbondy

Test Plan:
1. Import bookmarks
2. They should not appear in about:history until you visit them
@bbondy bbondy modified the milestones: 0.12.8dev, 0.12.9dev Nov 2, 2016
bbondy pushed a commit that referenced this issue Nov 2, 2016
2. Add creationTime field

fix #5183

Auditors: @bsclifton, @bbondy

Test Plan:
1. Import bookmarks
2. They should not appear in about:history until you visit them
@alexwykoff
Copy link
Contributor

Could not verify in OS X 0.12.8 RC1

screen shot 2016-11-04 at 1 03 41 pm

@srirambv
Copy link
Collaborator Author

srirambv commented Nov 4, 2016

On Windows too. Entry shows up in history suggestion but not shown in about:history

history1

cc: @darkdh

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.