Bookmark improvements #111

jsmitza · 2018-03-22T22:22:57Z

Hi, I love and-bible! I propose the following changes that will enhance the bookmark side with minimal GUI changes:

Display date bookmark added in bookmark list. (Added TwoLine2TitleListItem for this)
Display translation abbreviation in the bookmark list and show the text from that translation. (E.g. KJV and T4T)
Let a long press on bookmark list (after a bookmark was selected) select all bookmarks from previous long press to current long press. Useful to quickly select many bookmark entries for delete or labeling.
Show add bookmark button also if bookmark exist already. If selected the bookmark's date will be updated to the current time.

Note that the above require a change to the bookmark db table. In my tests it upgrades correctly (i.e. if you save bookmarks beforehand and then restore afterwards).

Regards
Jacques

…on book name in the bookmark list. (Does not display corresponding translation yet)

…p to that translation when selected.

… previous long click. (Can now quickly select multiple entries)

re-add. This will move bookmark up in date order list.

…en coming from old bookmarks.

tuomas2 · 2018-03-28T15:34:27Z

It would be helpful if you added screenshot or two to demonstrate the new feature.

jsmitza · 2018-03-29T09:58:16Z

Good suggestion, see below:

Edit: Note that the only visual change in the 2nd screen (from before) is that BOTH the add and remove buttons are shown at the top if a bookmarked verse (with the star) is re-selected.

jsmitza · 2018-03-29T10:21:49Z

As to the motivation for this change - see this from my feature request:

Minimal bookmark enhancement to support verse journal

Background: I don't have time/will to type notes in the app, but a chronological list of the verses that stood out for me during Bible reading helps me to meditate for longer on them as I revisit the list. It helps me see patterns on what God is saying to me over time. And Bible supports this to some extent already through bookmarks (thank you!) but it needs improvement.

ildar · 2018-03-29T10:35:43Z

wow, good point! I'd like to remind about quite similar issue: persistent history requests, see https://github.com/mjdenham/and-bible/pull/54 for details

tuomas2

I like the idea and suggest that this will be merged. Code looks OK and readable. There are some commented out code that needs to be cleaned up. See other small comments too.

(Please note that I'm not throughly familiar with the codebase but wanted to help with this code review. @mjdenham needs to check this too :) )

tuomas2 · 2018-03-29T14:10:09Z

and-bible/app/src/main/java/net/bible/android/control/bookmark/BookmarkControl.java

+//
+//						// create DTO with all required info to display this Translation text
+//						retval.add(new TranslationDto(book, text, fontFile));
+//					}


Did you forget here some commented-out code?

Some books may need a special font to display. For now I do not cater for this in the bookmark list - not sure that it is necessary. Your input on this will be welcomed.

I copied the corresponding code from CompareTranslationsControl (where the font is supported), but commented it out with a explanatory comment at the start. This can be taken out/modified depending on the way we want to go.

tuomas2 · 2018-03-29T14:12:11Z

and-bible/app/src/main/java/net/bible/android/control/bookmark/BookmarkControl.java

@@ -78,10 +81,12 @@ public boolean toggleBookmarkForVerseRange(VerseRange verseRange) {
 				} else {
 					Dialogs.getInstance().showErrorMsg(R.string.error_occurred);
 				}
-			} else {
+			}
+			if ((bookmarkDto ==null) || (menuItemId == R.id.add_bookmark)) {


code style (space before null)

tuomas2 · 2018-03-29T14:13:38Z

and-bible/app/src/main/java/net/bible/android/control/bookmark/BookmarkControl.java

+			bookUsed = bookmark.getBookUsed();
+		} catch (Exception e) {
+			Log.e(TAG, "Error getting book used", e);
+		}


Please be more specific with exception. Or is there a proper reason for catching everything?

I followed the pattern in other parts of the existing code (e.g in Bookmarks.java)

Ah, ok. This is considered generally bad habit but perhaps Martin has reason for this. At least error is logged properly.

tuomas2 · 2018-03-29T14:15:06Z

and-bible/app/src/main/java/net/bible/android/control/bookmark/BookmarkControl.java

+				//Book book = currentPageControl.getCurrentBible().getSwordDocumentFacade().getDocumentByInitials(abbrev);
+				Book book = currentBible.getSwordDocumentFacade().getDocumentByInitials(bookUsed);
+				if (book != null) {
+					verseText = swordContentFacade.getPlainText(book, bookmark.getConvertibleVerseRange().getVerseRange(((AbstractPassageBook) book).getVersification()));


Too long line, could you split? Also, readability could be improved by splitting some of this line into variables.

tuomas2 · 2018-03-29T14:20:36Z

and-bible/app/src/main/java/net/bible/android/control/bookmark/BookmarkControl.java

+
+			String bookUsed = bookmark.getBookUsed();
+			if ((bookUsed != null) && (!bookUsed.isEmpty()) && !bookUsed.equals(getCurrentBookUsed())) {
+				//Book book = currentPageControl.getCurrentBible().getSwordDocumentFacade().getDocumentByInitials(abbrev);


forgot comment?

tuomas2 · 2018-03-29T14:34:25Z

and-bible/app/src/main/java/net/bible/android/view/activity/bookmark/BookmarkItemAdapter.java

+            String bookUsed = bookmarkControl.getBookmarkBookUsed(item);
+            if (bookUsed != null && !bookUsed.isEmpty()) {
+                key += " " + bookUsed;
+            }


Tabs and spaces mixed in the code. I think the codebase uses tabs and not spaces in indentation. Could you fix that?

Check all other files that you have changed too

Note that though the codebase mostly use tabs as indents, it is not true everywhere (not my edits). If required I can change the indents for all the files I touched to be tabs.

Okay. Perhaps @mjdenham can comment on this. If this is the case, all files should be checked at some point and changed such that they are consistently indented everywhere.

I have tended to use tabs, but I can't insist on it.

tuomas2 · 2018-03-29T15:03:38Z

and-bible/app/src/main/java/net/bible/android/view/activity/bookmark/Bookmarks.java

+						int iStart = lastPos < position ? lastPos : position;
+						int iEnd = lastPos < position ? position : lastPos;
+						for (int i = iStart; i <= iEnd; i++) {
+							getListView().setItemChecked(i,true);//isItemChecked(lastPos));


clean up commented out code

tuomas2 · 2018-03-29T15:04:15Z

...p/src/main/java/net/bible/android/view/activity/page/actionmode/VerseActionModeMediator.java

@@ -165,7 +165,7 @@ public boolean onPrepareActionMode(ActionMode actionMode, Menu menu) {
 			// if start verse already bookmarked then enable Delete Bookmark menu item else Add Bookmark
 			Verse startVerse = getStartVerse();
 			boolean isVerseBookmarked = startVerse!=null && bookmarkControl.isBookmarkForKey(startVerse);
-			menu.findItem(R.id.add_bookmark).setVisible(!isVerseBookmarked);
+			menu.findItem(R.id.add_bookmark).setVisible(true);//!isVerseBookmarked);


Clean up commented out code

tuomas2 · 2018-03-29T15:04:44Z

and-bible/app/src/main/java/net/bible/android/view/util/widget/TwoLine2TitleListItem.java

+//        final TypedArray a = context.obtainStyledAttributes(
+//                attrs, R.styleable.TwoLineListItem);
+//
+//        a.recycle();


clean up commented out code. And as everything is commented out here, remove redundant function and in above altenrative constructor signatures replace this( with super(

This is a copy of the code in TwoLineListItem; if I change this the original should also be changed?

Hmm, then I guess here you could perhaps inherit this new class from TwoLineListItem? And I think you could consider improving TwoLineListItem code too as I suggested.

tuomas2 · 2018-03-29T15:11:34Z

and-bible/app/src/main/java/net/bible/android/control/bookmark/BookmarkControl.java

@@ -12,6 +12,7 @@
 import net.bible.android.control.page.CurrentBiblePage;
 import net.bible.android.control.page.CurrentPageManager;
 import net.bible.android.control.page.window.ActiveWindowPageManagerProvider;
+import net.bible.android.control.versification.ConvertibleVerseRange;


This is unused import

@jsmitza thanks for this proposal and the ability to not only make a great suggestion but to also make the commitment to code a solution. For me this is one of the key missing features that currently prevent me from using And Bible. In reality it would be nice if there were separate features for "hilighting" and "bookmarking" but your idea is a fine proposal to at least let bookmarks function as hilights.

mjdenham · 2018-04-13T19:53:21Z

Thanks for doing this. I love the idea of refreshing bookmarks.

Why do we need to save bookUsed? Bookmarks are not book specific.

mjdenham · 2018-04-13T19:56:35Z

Maybe you could split up the pull request into small parts. Maybe:

refresh bookmarks
show date
there seems to be more but I can't quite make it out because there is too much

Parts 1. and 2. should be fairly small pull requests which I can understand and accept quite quickly.

jsmitza · 2018-04-13T20:16:28Z

Why bookUsed: If you use different translations, you want to see your bookmark in the specific translation/language that you used to bookmark the verse. I read different translations (English, Afrikaans, T4T, etc) and often the specific wording in a specific translation is the one that touches my heart. When I review my bookmarks I want to see each bookmarked verse in the list in the translation that was used. See the screenshot at the beginning as an example. So bookUsed need to be saved.

jsmitza · 2018-04-13T20:24:59Z

Splitting Up: I am willing to do this, but it may not be soon, as I now have to work overtime for a month or so to make a deadline. (Which is why I am also slow to comment).

Note that the 4 items listed at the top are the only changes done. Part 2 might actually be more intrusive because it needs the database change.

mjdenham · 2018-04-13T21:26:54Z

Unfortunately making bookmarks book specific would not be something I would easily embrace. There are advantages to the bookmarks following whichever translation you are using.

mjdenham · 2018-04-13T21:27:22Z

Take plenty of time - I do!

jsmitza · 2018-04-14T10:34:50Z

What if
a) I make this an administration setting that you can turn on or off (and the default could be off as in the past): If turned on, bookmarks will show the corresponding translation and jump there if selected. If turned off it will not show the translation and follow the current translation. (Note that my changes will already show the bookmark in the current translation if no book was specified (or if it could not be found)). OR
b) We just add another button at the top of the bookmark list that toggle between the two modes mentioned in a)

Would this cover your reservations? I think b) is the best, but then I need an icon :-) It really is a dealbreaker for me to be able to see the actual history of my past encounters with God. (Often the specific wording of one translation can stand out for me, and I miss out on this if a single translation is used in the list). This is why I took the time to do the work. If you will embrace it if I add a) or b), then I plan to do the extra work.

mjdenham · 2018-04-14T20:22:37Z

I don't want to rush this in. I am not sure about it myself and don't like adding unnecessary complexity into the ui and the code.

You could start a thread to discuss this functionality in the And Bible discussion forum if you like.

jsmitza · 2018-04-15T11:21:17Z

Ok, I decided to spend a week-end day to try and split up as Martin suggested. Hope I do this right! I added 5 branches, one for each feature. They can be selected/merged individually as needed.

bookmark-showdate: Shows the date a bookmark was added in the bookmark list.
bookmark-update-date: Allows refreshing of an existing bookmark's date to current time by adding the selected verse again.
bookmark-select-many: Allows quick selection of many bookmarks in a list for labeling or deletion. Two long-presses will select all bookmarks between them.
bookgrid-underline-cur: Improve the highlight of the current book or chapter in the selection grid by underlining the name/chapter. (Why: Difficult to see current item otherwise, and it helps to quickly find the current book if you just want to change the chapter.)
bookmarklist-show-books-used: Add show-translation button at top of bookmark list (icon should probably be changed). Toggles between showing bookmarks in current translation, or each one in the original translation where it was bookmarked. Note that this change also involves a database upgrade (done automatically) as we now also need to save the book used by a bookmark.

I hope the above facilitates quicker trying/acceptance of some (if not all) of the changes.

tuomas2 · 2018-04-15T11:44:03Z

@jsmitza Can you create a new pull request out of each branch and then close this one?

jsmitza added 7 commits March 15, 2018 23:39

Display date in bookmark list

1fc5e08

Add book used to bookmark data in database and display the abbreviati…

bc5a594

…on book name in the bookmark list. (Does not display corresponding translation yet)

Forgot to up the DB version to 5

e5da38f

Show each bookmark in list in the translation it was selected and jum…

b076ac3

…p to that translation when selected.

Let long click on bookmarklist select all entries between current and…

b60ef34

… previous long click. (Can now quickly select multiple entries)

Show Add button on verse with existing bookmark; if selected delete and

f866e2a

re-add. This will move bookmark up in date order list.

Fixed: Did not test for current book = null case, causing problems wh…

e13dcd2

…en coming from old bookmarks.

tuomas2 requested changes Mar 29, 2018

View reviewed changes

jsmitza mentioned this pull request Apr 15, 2018

Display date bookmark was added in the bookmark list #118

Merged

jsmitza closed this Apr 15, 2018

This was referenced May 1, 2020

More flexible bookmark labels #638

Closed

Bookmark color doesn't appear when speak bookmark is on a verse... #535

Closed

Bookmark improvements #111

Bookmark improvements #111

Conversation

jsmitza commented Mar 22, 2018

tuomas2 commented Mar 28, 2018

jsmitza commented Mar 29, 2018 • edited Loading

jsmitza commented Mar 29, 2018

ildar commented Mar 29, 2018 via email • edited Loading

tuomas2 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjdenham commented Apr 13, 2018

mjdenham commented Apr 13, 2018 • edited Loading

jsmitza commented Apr 13, 2018

jsmitza commented Apr 13, 2018

mjdenham commented Apr 13, 2018

mjdenham commented Apr 13, 2018

jsmitza commented Apr 14, 2018 • edited Loading

mjdenham commented Apr 14, 2018

jsmitza commented Apr 15, 2018 • edited Loading

tuomas2 commented Apr 15, 2018

jsmitza commented Mar 29, 2018 •

edited

Loading

ildar commented Mar 29, 2018 via email •

edited

Loading

tuomas2 left a comment •

edited

Loading

mjdenham commented Apr 13, 2018 •

edited

Loading

jsmitza commented Apr 14, 2018 •

edited

Loading

jsmitza commented Apr 15, 2018 •

edited

Loading