Merge lp:~hegarty-sam/midori/midori-inline-speeddialtitle into lp:midori

Proposed by Sam Hegarty
Status: Superseded
Proposed branch: lp:~hegarty-sam/midori/midori-inline-speeddialtitle
Merge into: lp:midori
Diff against target: 162 lines (+62/-14)
2 files modified
data/speeddial-head.html (+61/-12)
midori/midori-speeddial.vala (+1/-2)
To merge this branch: bzr merge lp:~hegarty-sam/midori/midori-inline-speeddialtitle
Reviewer Review Type Date Requested Status
Sam Hegarty (community) Needs Resubmitting
Paweł Forysiuk Needs Fixing
Danielle Foré (community) Needs Fixing
Review via email: mp+237209@code.launchpad.net

This proposal has been superseded by a proposal from 2014-11-08.

Description of the change

Change how the user edits the title of shortcuts on the speed dial so instead of a prompt displaying it's done inline.

To post a comment you must log in.
6815. By Sam Hegarty

update comments

6816. By Sam Hegarty

keydown appears to be consistent/correct behaviour

Revision history for this message
Sam Hegarty (hegarty-sam) :
review: Approve
Revision history for this message
Danielle Foré (danrabbit) wrote :

Is there a reason for changing the indentation on diffline 9?

needs a space after the : on diffline 17

Maybe use "transparent" instead of "rgba(0, 0, 0, 0)"

I can confirm that this fixes the issue :)

review: Needs Fixing
6817. By Sam Hegarty

use css transparent instead of rgba(0,0,0,0) and fix indentation

6818. By Sam Hegarty

few code-style issues

Revision history for this message
Sam Hegarty (hegarty-sam) wrote :

Woops, didn't notice I'd changed the indentation because of my editor settings. Fixed the other bits as well. Thanks for taking a look!

Revision history for this message
Paweł Forysiuk (tuxator) wrote :

Please don't set review status yourself. Reviewer will do it for you once review is done.

Maybe i am missing something obvious, but there seems to be no way now to cancel the edit.
If you fat-finger or delete something you did not want to you seem to stuck with it now.
Also you may not remember what was there before the edit.
Seems like whatever was in the edit entry you are stuck with now.

First thing i would try as a user to cancel the edit would be hitting esc or clicking somewhere away from the dial tile (maybe highlight subtly where the given tile ends in the title edit mode?)

Other than that it looks mostly fine from the quick glance.

review: Needs Fixing
Revision history for this message
Sam Hegarty (hegarty-sam) wrote :

Ah good point. ctrl+z can be used to undo but it would be nice to have a proper way to cancel. What about an 'x' inside the text-box to click?

Revision history for this message
Danielle Foré (danrabbit) wrote :

I don't think there really needs to be a graphical way to cancel the change as long as there is an undo. There's no cancel when you're renaming in the file browser and I've never heard of this being a major issue people have with file browsers.

Adding 'esc' to cancel would probably be fine. I would personally expect the box losing focus (by clicking outside it for example) to save the change, not to cancel it.

6819. By Sam Hegarty

pressing escape now cancels editing title

Revision history for this message
Sam Hegarty (hegarty-sam) wrote :

Alright, I've made pressing escape reset the input to what it was when they started changing it.

6820. By Sam Hegarty

rename js function to make more sense

Revision history for this message
Sam Hegarty (hegarty-sam) :
review: Needs Resubmitting
6821. By Sam Hegarty

Clicking off input box returns input to previous value

6822. By Sam Hegarty

Undo previous change

Revision history for this message
Sam Hegarty (hegarty-sam) wrote :

Realized after commiting I'd read your comment wrong. Woops.

6823. By Sam Hegarty

Update with changes from main branch

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'data/speeddial-head.html'
2--- data/speeddial-head.html 2013-06-25 16:12:25 +0000
3+++ data/speeddial-head.html 2014-10-28 01:01:55 +0000
4@@ -27,7 +27,7 @@
5 cursor: default;
6 font-size: 13px;
7 color: #4d4d4d;
8- -webkit-user-select: none;
9+ -webkit-user-select: none;
10 }
11
12 html, body {
13@@ -85,8 +85,13 @@
14 }
15
16 .title {
17+ background: transparent;
18+ border: 1px solid transparent;
19+ display: block;
20 text-align: center;
21 margin: 8px;
22+ margin-left: auto;
23+ margin-right: auto;
24 white-space: nowrap;
25 text-overflow: ellipsis;
26 overflow: hidden;
27@@ -119,9 +124,24 @@
28 outline: 1px dotted black;
29 background-color: #eef;
30 }
31+
32+ .selectable {
33+ -webkit-user-select: text;
34+ }
35 </style>
36
37 <script type="text/javascript">
38+ var previousName = "";
39+
40+ function input_key_down (ev) {
41+ // 13 is the key code for enter
42+ if(ev.keyCode == 13) ev.target.blur();
43+ // 27 is the key code for escape
44+ if(ev.keyCode == 27) {
45+ ev.target.value = previousName;
46+ ev.target.blur();
47+ }
48+ }
49
50 function add_tile (ev) {
51 ev.preventDefault();
52@@ -137,17 +157,26 @@
53 console.log ("speed_dial-save-add " + id + " " + url);
54 }
55
56- function rename_tile (ev) {
57- var old_name = ev.target.textContent;
58-
59- var name = prompt ("{enter_shortcut_name}", old_name);
60- if (!name)
61- return;
62-
63+ function done_editing_title (ev) {
64+ input = ev.target;
65+ input.style.backgroundColor = "transparent";
66+ input.style.border = "1px solid transparent";
67+
68+ var name = ev.target.value;
69+ if (name == "")
70+ name = previousName;
71 var id = ev.target.parentNode.id;
72 console.log ("speed_dial-save-rename " + id + " " + name);
73 }
74
75+ function editing_tile (ev) {
76+ input = ev.target;
77+ previousName = input.value;
78+ // indicate to user they are editing the title
79+ input.style.backgroundColor = "#F8F8F8";
80+ input.style.border = "1px solid #808060";
81+ }
82+
83 function delete_tile (ev) {
84 ev.preventDefault();
85
86@@ -158,7 +187,6 @@
87 console.log ("speed_dial-save-delete " + id);
88 }
89
90-
91 var firstNode, secondNode;
92 var cursor;
93
94@@ -177,6 +205,23 @@
95 if (ev == undefined)
96 return;
97
98+ /* If the target of the event is an element of class title
99+ * and not also of class add-shortcut we are editing title
100+ */
101+ var classes = ev.target.className.split(" ");
102+ var edit_title = false;
103+ for (var i = 0; i < classes.length; i++) {
104+ if(classes[i] == "add-shortcut") {
105+ edit_title = false;
106+ break;
107+ }
108+ if(classes[i] == "title")
109+ edit_title = true;
110+ }
111+
112+ if(edit_title)
113+ return;
114+
115 ev.preventDefault();
116 var ele = ev.target;
117 cursor = ele.style.cursor;
118@@ -198,7 +243,8 @@
119 var eparent = get_dial_div (ele);
120
121 ele.style.cursor = cursor;
122- secondNode = eparent.id;
123+ if(eparent != undefined) secondNode = eparent.id;
124+ else return;
125
126 /* ommit just mere clicking the dial */
127 if (firstNode != secondNode && firstNode != undefined)
128@@ -237,8 +283,11 @@
129 var titles = document.getElementsByClassName ("title");
130 var len = titles.length;
131 for (var i = 0; i < len; i++) {
132- if (titles[i].parentNode.childNodes[0].className != "preview new")
133- titles[i].addEventListener ('click', rename_tile, false);
134+ if (titles[i].parentNode.childNodes[0].className != "preview new") {
135+ titles[i].addEventListener ('click', editing_tile, false);
136+ titles[i].addEventListener ('blur', done_editing_title, false);
137+ titles[i].addEventListener ('keydown', input_key_down, false);
138+ }
139 }
140
141 var crosses = document.getElementsByClassName ("cross");
142
143=== modified file 'midori/midori-speeddial.vala'
144--- midori/midori-speeddial.vala 2013-10-28 23:08:02 +0000
145+++ midori/midori-speeddial.vala 2014-10-28 01:01:55 +0000
146@@ -188,7 +188,6 @@
147 string header = head.replace ("{title}", _("Speed Dial")).
148 replace ("{click_to_add}", _("Click to add a shortcut")).
149 replace ("{enter_shortcut_address}", _("Enter shortcut address")).
150- replace ("{enter_shortcut_name}", _("Enter shortcut title")).
151 replace ("{are_you_sure}", _("Are you sure you want to delete this shortcut?"));
152 var markup = new StringBuilder (header);
153
154@@ -251,7 +250,7 @@
155 <div class="shortcut" id="%u"><div class="preview">
156 <a class="cross" href="#"></a>
157 <a href="%s"><img src="data:image/png;base64,%s" title='%s'></a>
158- </div><div class="title">%s</div></div>
159+ </div><input type="text" class="title selectable" value="%s"></div>
160 """,
161 slot, uri, encoded ?? "", title, title ?? "");
162 }

Subscribers

People subscribed via source and target branches

to all changes: