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

Remove customTitle from siteDetail object #6585

Closed
bsclifton opened this issue Jan 9, 2017 · 14 comments
Closed

Remove customTitle from siteDetail object #6585

bsclifton opened this issue Jan 9, 2017 · 14 comments

Comments

@bsclifton
Copy link
Member

bsclifton commented Jan 9, 2017

Test plan

#10136 (comment)


Discussion starts here #6108 (comment)

To keep things simple, I'd like to propose that we eliminate custom title.

  • When a bookmark is added for a page, it will default to the title of the page
  • Once set, the title for that bookmark will never be updated again

Removing this should resolve the following issues:
#6104
#6108
#3694

There are likely more which have this as the root cause (we can search for issues where titles change)

@bsclifton bsclifton assigned bsclifton and unassigned bsclifton Jan 9, 2017
@NejcZdovc NejcZdovc self-assigned this Jan 9, 2017
@bbondy
Copy link
Member

bbondy commented Jan 9, 2017

I think removing custom title is wrong.
If you did...
If you rename a bookmark, then you visit a site do you update the title attribute?
If you don't update the title attribute, what if the website title changes?

@bbondy
Copy link
Member

bbondy commented Jan 9, 2017

I think if we did we would have to split out bookmarks from sites at the same time

@bbondy
Copy link
Member

bbondy commented Jan 9, 2017

I think we should do it but I think we should split out bookmarks first or at the same time.

@bsclifton
Copy link
Member Author

@bbondy from the original discussion (sorry, should have posted this here too- @bradleyrichter had the same concern)

@bradleyrichter my proposal is to eliminate the custom title field and then use the regular title field like a custom title. Right now, the title is something that users can't change... but it causes problems because it will update if the page title updates. The user would have to go and explicitly set a title if they didn't want it to change

I think it's a better experience to always treat the title like it's a custom title. It should never be updated unless the person changes it themselves. That should be consistent with other browsers. What do you think?

@bsclifton
Copy link
Member Author

bsclifton commented Jan 9, 2017

@bbondy the title updating itself is what causes several bugs unfortunately. For example, let's say the site ends up doing a redirect via JavaScript with a simple "we've moved" page. In this case, it'll rename the page "We've moved! Redirecting you now to so and so.com...."

I tested with Safari and Chrome briefly and once a bookmark is saved, the title did not seem to be updated when visited again (if the page title was updated)

@bbondy
Copy link
Member

bbondy commented Jan 9, 2017

If the title of a bookmark is updating currently on navigation then a bug was introduced, this used to work correctly. Still does as far as I can tell from testing just now.

I'm ok and in favour of getting rid of customTitle, it's only wrong if done without any other changes. It should be done with splitting out bookmarks from within the sites storage. This will also solve problems with the upgrade scenario if we simply stopped using customTitle. It will also simplify the logic in various places.

  • visitedSites: sites that have no tags
  • bookmarks: sites that have bookmark or bookmark-folder tags, they need to be stored together because you can reorder them on the bookmarks toolbar
  • pinnedSites: sites with pinned tag

The reader tag describe in docs/state.md is not used so can be ignored.

@bbondy
Copy link
Member

bbondy commented Jan 9, 2017

Also you can still use the siteUtils for each of these but just store them differently in app storage.
Upgrade scenario can just check if appState has a sites, and if so then do the upgrade steps to split them. App would then use the 3 new categories directly and not sites.

@bsclifton
Copy link
Member Author

bsclifton commented Jan 9, 2017

Great points @bbondy 😄

@NejcZdovc you can check the documention by @cezaraugusto for our existing "Site Detail" objects and behavior here:
https://github.com/brave/browser-laptop/wiki/Overview-of-the-siteDetail-object

And @darkdh has modified that behavior further with #5587 (making each entry a hash instead, which is unique; PR is still open- pls feel free to review). Like @bbondy is saying, this is a great chance to remove bookmarks from the sites object 😄

Let's talk more on Slack if you have questions or want to talk about a design (we can then document any findings here)

@NejcZdovc
Copy link
Contributor

Blocked by #5587

@ayumi
Copy link
Contributor

ayumi commented Jan 11, 2017

I think splitting sites out is a good idea. We'll need to update Sync code to match the new schema.

cc @diracdeltas

@diracdeltas
Copy link
Member

i agree

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Jan 28, 2017
Resolves brave#6585

Auditors:
@bbondy, @ayumi, @diracdeltas, @bsclifton

Test Plan:
- everything should work the same as before
@NejcZdovc NejcZdovc removed their assignment Mar 9, 2017
@luixxiul
Copy link
Contributor

luixxiul commented Jun 5, 2017

@NejcZdovc is this issue still blocked on something? #6585 (comment)

@luixxiul luixxiul added the needs-info Another team member needs information from the PR/issue opener. label Jun 5, 2017
@NejcZdovc
Copy link
Contributor

No it's not blocked anymore

@luixxiul luixxiul removed the needs-info Another team member needs information from the PR/issue opener. label Jun 6, 2017
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Jul 18, 2017
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Jul 18, 2017
Resovles brave#9978
Resolves brave#6108
Resovles brave#6585
Resolves brave#6104
Resolves brave#3694

Auditors:

Test Plan:
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Jul 26, 2017
Resolves brave#1856
Resolves brave#2771
Resolves brave#3646
Resolves brave#3694
Resolves brave#4224
Resolves brave#4260
Resolves brave#4833
Resolves brave#4868
Resolves brave#4929
Resolves brave#5699
Resolves brave#6104
Resolves brave#6108
Resolves brave#6585
Resolves brave#8022
Resolves brave#9301
Resolves brave#9326
Resolves brave#9978
Resolves brave#10026

Auditors:

Test Plan:
NejcZdovc added a commit that referenced this issue Jul 27, 2017
Resolves #1856
Resolves #2771
Resolves #3646
Resolves #3694
Resolves #4224
Resolves #4260
Resolves #4833
Resolves #4868
Resolves #4929
Resolves #5699
Resolves #6104
Resolves #6108
Resolves #6585
Resolves #8022
Resolves #9301
Resolves #9326
Resolves #9978
Resolves #10026

Auditors:

Test Plan:
NejcZdovc added a commit that referenced this issue Jul 27, 2017
Resolves #1856
Resolves #2771
Resolves #3646
Resolves #3694
Resolves #4224
Resolves #4260
Resolves #4833
Resolves #4868
Resolves #4929
Resolves #5699
Resolves #6104
Resolves #6108
Resolves #6585
Resolves #8022
Resolves #9301
Resolves #9326
Resolves #9978
Resolves #10026

Auditors:

Test Plan:
NejcZdovc added a commit that referenced this issue Jul 27, 2017
Resolves #1856
Resolves #2771
Resolves #3646
Resolves #3694
Resolves #4224
Resolves #4260
Resolves #4833
Resolves #4868
Resolves #4929
Resolves #5699
Resolves #6104
Resolves #6108
Resolves #6585
Resolves #8022
Resolves #9301
Resolves #9326
Resolves #9978
Resolves #10026

Auditors:

Test Plan:
NejcZdovc added a commit that referenced this issue Jul 28, 2017
Resolves #1856
Resolves #2771
Resolves #3646
Resolves #3694
Resolves #4224
Resolves #4260
Resolves #4833
Resolves #4868
Resolves #4929
Resolves #5699
Resolves #6104
Resolves #6108
Resolves #6585
Resolves #8022
Resolves #9301
Resolves #9326
Resolves #9978
Resolves #10026

Auditors:

Test Plan:
NejcZdovc added a commit that referenced this issue Jul 28, 2017
Resolves #1856
Resolves #2771
Resolves #3646
Resolves #3694
Resolves #4224
Resolves #4260
Resolves #4833
Resolves #4868
Resolves #4929
Resolves #5699
Resolves #6104
Resolves #6108
Resolves #6585
Resolves #8022
Resolves #9301
Resolves #9326
Resolves #9978
Resolves #10026

Auditors:

Test Plan:
NejcZdovc added a commit that referenced this issue Jul 28, 2017
Resolves #1856
Resolves #2771
Resolves #3646
Resolves #3694
Resolves #4224
Resolves #4260
Resolves #4833
Resolves #4868
Resolves #4929
Resolves #5699
Resolves #6104
Resolves #6108
Resolves #6585
Resolves #8022
Resolves #9301
Resolves #9326
Resolves #9978
Resolves #10026

Auditors:

Test Plan:
NejcZdovc added a commit that referenced this issue Jul 31, 2017
Resolves #1856
Resolves #2771
Resolves #3646
Resolves #3694
Resolves #4224
Resolves #4260
Resolves #4833
Resolves #4868
Resolves #4929
Resolves #5699
Resolves #6104
Resolves #6108
Resolves #6585
Resolves #8022
Resolves #9301
Resolves #9326
Resolves #9978
Resolves #10026

Auditors:

Test Plan:
NejcZdovc added a commit that referenced this issue Jul 31, 2017
Resolves #1856
Resolves #2771
Resolves #3646
Resolves #3694
Resolves #4224
Resolves #4260
Resolves #4833
Resolves #4868
Resolves #4929
Resolves #5699
Resolves #6104
Resolves #6108
Resolves #6585
Resolves #8022
Resolves #9301
Resolves #9326
Resolves #9978
Resolves #10026

Auditors:

Test Plan:
NejcZdovc added a commit that referenced this issue Aug 1, 2017
Resolves #1856
Resolves #2771
Resolves #3646
Resolves #3694
Resolves #4224
Resolves #4260
Resolves #4833
Resolves #4868
Resolves #4929
Resolves #5699
Resolves #6104
Resolves #6108
Resolves #6585
Resolves #8022
Resolves #9301
Resolves #9326
Resolves #9978
Resolves #10026

Auditors:

Test Plan:
NejcZdovc added a commit that referenced this issue Aug 3, 2017
Resolves #1856
Resolves #2771
Resolves #3646
Resolves #3694
Resolves #4224
Resolves #4260
Resolves #4833
Resolves #4868
Resolves #4929
Resolves #5699
Resolves #6104
Resolves #6108
Resolves #6585
Resolves #8022
Resolves #9301
Resolves #9326
Resolves #9978
Resolves #10026

Auditors:

Test Plan:
@NejcZdovc NejcZdovc modified the milestones: 0.21.x (Nightly Channel), 0.20.x (Developer Channel) Aug 4, 2017
NejcZdovc added a commit that referenced this issue Aug 4, 2017
Resolves #1646
Resolves #1856
Resolves #2655
Resolves #2771
Resolves #3646
Resolves #3694
Resolves #4224
Resolves #4260
Resolves #4833
Resolves #4868
Resolves #4929
Resolves #5072
Resolves #5699
Resolves #5382
Resolves #6104
Resolves #6108
Resolves #6585
Resolves #8022
Resolves #9301
Resolves #9326
Resolves #9978
Resolves #10026

Auditors:

Test Plan:
dfperry5 pushed a commit to dfperry5/browser-laptop that referenced this issue Aug 18, 2017
Resolves brave#1646
Resolves brave#1856
Resolves brave#2655
Resolves brave#2771
Resolves brave#3646
Resolves brave#3694
Resolves brave#4224
Resolves brave#4260
Resolves brave#4833
Resolves brave#4868
Resolves brave#4929
Resolves brave#5072
Resolves brave#5699
Resolves brave#5382
Resolves brave#6104
Resolves brave#6108
Resolves brave#6585
Resolves brave#8022
Resolves brave#9301
Resolves brave#9326
Resolves brave#9978
Resolves brave#10026

Auditors:

Test Plan:
@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.