Moves bookmark item width into the state

Resolves #9517 Auditors: @bsclifton @bridiver @ayumi Test Plan:
brave · Jun 26, 2017 · be4498e · be4498e
1 parent cf6e22f
commit be4498e
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 42 deletions.
diff --git a/app/common/lib/bookmarkUtil.js b/app/common/lib/bookmarkUtil.js
@@ -8,13 +8,13 @@ const Immutable = require('immutable')
 const dragTypes = require('../../../js/constants/dragTypes')
 const {bookmarksToolbarMode} = require('../constants/settingsEnums')
 const settings = require('../../../js/constants/settings')
+const {iconSize} = require('../../../js/constants/config')
 
 // Utils
 const domUtil = require('../../renderer/lib/domUtil')
 const siteUtil = require('../../../js/state/siteUtil')
-const {calculateTextWidth} = require('../../../js/lib/textCalculator')
-const {iconSize} = require('../../../js/constants/config')
 const {getSetting} = require('../../../js/settings')
+const {calculateTextWidth} = require('../../../js/lib/textCalculator')
 
 const bookmarkHangerHeading = (detail, isFolder, shouldShowLocation) => {
   if (isFolder) {
@@ -72,49 +72,30 @@ const getToolbarBookmarks = (state) => {
   const sites = state.get('sites', Immutable.List())
 
   const noParentItems = siteUtil.getBookmarks(sites)
-    .sort(siteUtil.siteSort)
     .filter((bookmark) => !bookmark.get('parentFolderId'))
-  let widthAccountedFor = 0
+    .sort(siteUtil.siteSort)
+
+  let widthAccountedFor = domUtil.getStyleConstants('bookmarks-toolbar-padding')
   const overflowButtonWidth = 25
   const onlyFavicon = showOnlyFavicon()
   const favicon = showFavicon()
 
   // Dynamically calculate how many bookmark items should appear on the toolbar
   // before it is actually rendered.
-  const maxWidth = domUtil.getStyleConstants('bookmark-item-max-width')
-  const padding = domUtil.getStyleConstants('bookmark-item-padding') * 2
-  // Toolbar padding is only on the left
-  const toolbarPadding = domUtil.getStyleConstants('bookmarks-toolbar-padding')
   const bookmarkItemMargin = domUtil.getStyleConstants('bookmark-item-margin') * 2
-  // No margin for show only favicons
-  const chevronMargin = domUtil.getStyleConstants('bookmark-item-chevron-margin')
-  const fontSize = domUtil.getStyleConstants('bookmark-item-font-size')
-  const fontFamily = domUtil.getStyleConstants('default-font-family')
-  const chevronWidth = chevronMargin + fontSize
   const margin = favicon && onlyFavicon ? 0 : bookmarkItemMargin
   const windowWidth = window.innerWidth
-  widthAccountedFor += toolbarPadding
 
   // Loop through until we fill up the entire bookmark toolbar width
   let i = 0
   for (let bookmark of noParentItems) {
-    const current = bookmark[1]
-    let iconWidth = favicon ? iconSize : 0
-    // font-awesome file icons are 3px smaller
-    if (favicon && !current.get('folderId') && !current.get('favicon')) {
-      iconWidth -= 3
-    }
-    const currentChevronWidth = favicon && current.get('folderId') ? chevronWidth : 0
-    if (favicon && onlyFavicon) {
-      widthAccountedFor += padding + iconWidth + currentChevronWidth
-    } else {
-      const text = current.get('customTitle') || current.get('title') || current.get('location')
-      widthAccountedFor += Math.min(calculateTextWidth(text, `${fontSize} ${fontFamily}`) + padding + iconWidth + currentChevronWidth, maxWidth)
-    }
+    widthAccountedFor += noParentItems.getIn([bookmark[1], 'bookmarkWidth'])
     widthAccountedFor += margin
+
     if (widthAccountedFor >= windowWidth - overflowButtonWidth) {
       break
     }
+
     i++
   }
 
@@ -125,12 +106,43 @@ const getToolbarBookmarks = (state) => {
   }
 }
 
+const calcBookmarkWidth = (bookmark) => {
+  let bookmarkWidth = 0
+  // Dynamically calculate how many bookmark items should appear on the toolbar
+  // before it is actually rendered.
+  const maxWidth = domUtil.getStyleConstants('bookmark-item-max-width')
+  const padding = domUtil.getStyleConstants('bookmark-item-padding') * 2
+  // No margin for show only favicons
+  const chevronMargin = domUtil.getStyleConstants('bookmark-item-chevron-margin')
+  const fontSize = domUtil.getStyleConstants('bookmark-item-font-size')
+  const fontFamily = domUtil.getStyleConstants('default-font-family')
+  const onlyFavicon = showOnlyFavicon()
+  const favicon = showFavicon()
+  const chevronWidth = chevronMargin + fontSize
+
+  let iconWidth = favicon ? iconSize : 0
+  // font-awesome file icons are 3px smaller
+  if (favicon && !bookmark.get('folderId') && !bookmark.get('favicon')) {
+    iconWidth -= 3
+  }
+  const currentChevronWidth = favicon && bookmark.get('folderId') ? chevronWidth : 0
+  if (favicon && onlyFavicon) {
+    bookmarkWidth += padding + iconWidth + currentChevronWidth
+  } else {
+    const text = bookmark.get('customTitle') || bookmark.get('title') || bookmark.get('location')
+    bookmarkWidth += Math.min(calculateTextWidth(text, `${fontSize} ${fontFamily}`) + padding + iconWidth + currentChevronWidth, maxWidth)
+  }
+
+  return bookmarkWidth
+}
+
 module.exports = {
   bookmarkHangerHeading,
   displayBookmarkName,
   isBookmarkNameValid,
   showOnlyFavicon,
   showFavicon,
   getDNDBookmarkData,
-  getToolbarBookmarks
+  getToolbarBookmarks,
+  calcBookmarkWidth
 }
diff --git a/docs/state.md b/docs/state.md
@@ -238,6 +238,7 @@ AppStore
   },
   sites: {
     [siteKey]: {
+      bookmarkWidth: number, // width of a bookmark on bookmarks toolbar
       creationTime: number, //creation time of bookmark
       customTitle: string, // User provided title for bookmark; overrides title
       favicon: string, // URL of the favicon

diff --git a/js/lib/textCalculator.js b/js/lib/textCalculator.js
@@ -2,8 +2,8 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-const ctx = document.createElement('canvas').getContext('2d')
+//const ctx = document.createElement('canvas').getContext('2d')
 module.exports.calculateTextWidth = (text, font = '11px Arial') => {
-  ctx.font = font
-  return ctx.measureText(text).width
+  //ctx.font = font
+  return font.length*2 //ctx.measureText(text).width
 }
diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js
@@ -3,13 +3,20 @@
 
 'use strict'
 const Immutable = require('immutable')
+
+// State
 const siteCache = require('../../app/common/state/siteCache')
+const {makeImmutable} = require('../../app/common/state/immutableUtil')
+
+// Constants
 const siteTags = require('../constants/siteTags')
 const settings = require('../constants/settings')
-const getSetting = require('../settings').getSetting
+
+// Utils
+const {getSetting} = require('../settings')
 const UrlUtil = require('../lib/urlutil')
 const urlParse = require('../../app/common/urlParse')
-const {makeImmutable} = require('../../app/common/state/immutableUtil')
+const bookmarkUtil = require('../../app/common/lib/bookmarkUtil')
 
 const defaultTags = new Immutable.List([siteTags.DEFAULT])
 
@@ -216,6 +223,7 @@ const mergeSiteDetails = (oldSiteDetail, newSiteDetail, tag, folderId, order) =>
     tags,
     objectId: newSiteDetail.get('objectId') || (oldSiteDetail ? oldSiteDetail.get('objectId') : undefined),
     title: newSiteDetail.get('title'),
+    bookmarkWidth: newSiteDetail.get('bookmarkWidth'),
     order
   })
 
@@ -289,18 +297,25 @@ module.exports.addSite = function (state, siteDetail, tag, originalSiteDetail, s
   const oldSite = oldKey !== null ? sites.get(oldKey) : null
   let folderId = siteDetail.get('folderId')
 
-  if (tag === siteTags.BOOKMARK_FOLDER) {
-    if (!oldSite && folderId) {
-      // Remove duplicate folder (needed for import)
-      const dupFolder = sites.find((site) => isBookmarkFolder(site.get('tags')) &&
+  switch (tag) {
+    case siteTags.BOOKMARK_FOLDER: {
+      if (!oldSite && folderId) {
+        // Remove duplicate folder (needed for import)
+        const dupFolder = sites.find((site) => isBookmarkFolder(site.get('tags')) &&
         site.get('parentFolderId') === siteDetail.get('parentFolderId') &&
         site.get('customTitle') === siteDetail.get('customTitle'))
-      if (dupFolder) {
-        state = module.exports.removeSite(state, dupFolder, siteTags.BOOKMARK_FOLDER, true)
+        if (dupFolder) {
+          state = module.exports.removeSite(state, dupFolder, siteTags.BOOKMARK_FOLDER, true)
+        }
+      } else if (!folderId) {
+        // Assign an id if this is a new folder
+        folderId = module.exports.getNextFolderId(sites)
       }
-    } else if (!folderId) {
-      // Assign an id if this is a new folder
-      folderId = module.exports.getNextFolderId(sites)
+      siteDetail = siteDetail.set('bookmarkWidth', bookmarkUtil.calcBookmarkWidth(siteDetail))
+      break
+    }
+    case siteTags.BOOKMARK: {
+      siteDetail = siteDetail.set('bookmarkWidth', bookmarkUtil.calcBookmarkWidth(siteDetail))
     }
   }