Location does not appear in multiday view event box

RESOLVED FIXED in 6.2

Status

defect
RESOLVED FIXED
14 years ago
Last year

People

(Reporter: gekacheka, Assigned: clemens)

Tracking

({late-l10n, regression})

Dependency tree / graph
Bug Flags:
wanted-calendar0.9 -

Details

(Whiteboard: [has l10n impact])

Attachments

(7 attachments, 10 obsolete attachments)

94.46 KB, text/calendar
Details
39.77 KB, image/png
Details
64.93 KB, image/png
Details
36.07 KB, text/calendar
Details
8.49 KB, text/plain
Details
84.56 KB, patch
Details | Diff | Splinter Review
12.84 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: trunk 20051224

Previously day and week view event boxes contained
  Title (location)
  descriptionLine1
  descriptionLine2...

With new views, the contain only
  Title

Reproducible: Always

Steps to Reproduce:
1. Create a multi-hour event with title, location, and description.
2. view in day view or week view (new views)


Actual Results:  
  Title

(and lots of empty space)

Expected Results:  
  Title (location)
  descriptionLine1
  descriptionLine2...


how old week view added event box text
http://lxr.mozilla.org/mozilla/source/calendar/resources/content/weekView.js#464
Blocks: 321164
Keywords: regression
(patch -l -p 2 -i file.patch)

Separates the display <description> / edit <textbox> into calendar-item-field-textbox, and uses it for the title, location, and description.  Title and location are zero or one line each, wrappable if long.  Description takes the remaining space, wrappable.  Calendar-item-field-textbox has a minimum height of 2 pixels so that if there is no location, it won't take much space, but will still be clickable so a location can be added.

This bug was only a regression in sunbird.  This patch assumes that it is ok to add this feature to lightning as well.  

Adding the additional info probably has a speed tradeoff.  I tried to use the same techniques (use description for display, and hide textbox unless editing), but paired them in a deck.

(One minor issue I haven't been able to resolve:  in day view, a long description paragraph is wrapped to width of the longest word, as if it is originally laid out for a zero-width event box, and not re-wrapped for the final width.  I'm assuming this is a layout bug.  Jiggling the width of the pane causes it to re-wrap correctly.)
Attachment #206859 - Flags: first-review?(mvl)
I'm not sure if we really want to show all the info. I'm afraid it will look very crowded, and you won't be able to find the information you are looking for.
And besides, the location and description are (or should be) in the tooltip.
Can you provide screenshots of the day and weekviews? (with some reasonable lenght events, like 1 hour)
In the week view (multiday view) the description should not be shown by default IMO, only the location. The description can become very long and unless you have very long events which do not collide which other events at the same time, you won't see the description most of the time.

I'm fine with it in the day view, because we have much more space there.
Assignee: mostafah → gekacheka
I think we should only show the location. The description will be too long, and look too cluttered. (Even in day view. There might be more space, but that's only horizontal space. A description of a few lines, and it's very busy again)
But let's ask beltzner...
Yeah, the description info seems like overkill. The location should probably also only be shown on the day view, not the week or month views. Ideally these things will all be skinnable though, right?
I can agree the description could be optional.
I think the option should be controlled by the user as an individual preference, not tied to an overall theme.  (There is no such thing as a theme extension, is there?)

Showing location in week view is useful to notice where there might be bad transitions between events that are far apart in space, far enough ahead of time that one might be rescheduled or shortened if necessary.
(In reply to comment #9)
> I can agree the description could be optional. I think the option should be 
> controlled by the user as an individual preference, not tied to an overall 
> theme.  (There is no such thing as a theme extension, is there?)

Don't go the pref route! We have seen what will happen when that happens with the Suite. I like the skinning suggestion very much. Let's just add a display:none; for the description in the day and week view CSS so that themers can change that as well as individuals via userchrome.css

> Showing location in week view is useful to notice where there might be bad
> transitions between events that are far apart in space, far enough ahead of
> time that one might be rescheduled or shortened if necessary.

I tend to agree with that suggestion, though I'm not the UI expert here.
Component: Sunbird and Calendar-Extension Front End → Base
*** Bug 330390 has been marked as a duplicate of this bug. ***
The bugspam monkeys have been set free and are feeding on Calendar :: Internal Components. Be afraid for your sanity!
QA Contact: sunbird → base
*** Bug 360909 has been marked as a duplicate of this bug. ***
Comment on attachment 206859 [details] [diff] [review]
v1. add location and description to multiday-view event box

Moving first-review to Stefan, since mvl's queue is pretty big and he can only cope with his ui-reviews and 2nd reviews.
Attachment #206859 - Flags: ui-review?(mvl)
Attachment #206859 - Flags: first-review?(ssitter)
Attachment #206859 - Flags: first-review?(mvl)
Comment on attachment 206859 [details] [diff] [review]
v1. add location and description to multiday-view event box

ui-review=mvl on adding the location. You can add the description, but make is default to hidden using CSS. Then users can enable it (maybe using a very simple extension)
Attachment #206859 - Flags: ui-review?(mvl) → ui-review+
Comment on attachment 206859 [details] [diff] [review]
v1. add location and description to multiday-view event box

patch no longer applies, due to many changes to underlying code.
Attachment #206859 - Attachment is obsolete: true
Attachment #206859 - Flags: first-review?(ssitter)
(patch -l -p 2 - i file.patch)

Result:
  In week view, events display Location (if tall enough)
  In Day view, events display Location and Description (if tall enough)
  In both these views, all-day events display location (if wide enough)
These will be more useful once hour height can be enlarged by the user.

Other Bugs fixed:
* Now possible to insert cursor in Month view event titles.
  1. click on event in month view to select event,
  2. click on title of selected event to edit it (becomes selected)
  3. click between characters of selected title to place cursor.
  (click in step 3 previously ended edit).
* Now possible to control-click (command-click on MacOS) to toggle selection.
  Clicking with toggle modifier never starts to edit text.
  Clicking without modifier starts to edit text only if previously selected.
  Double-click and context-click still work.

Minor problems:
* When the location field is empty, it can be difficult to click to
  edit it.  This is partly a layout problem, at least in in all-day
  events in multiday-view, where the empty box has ample min-height
  and min-width, but it is laid out below the baseline from where you
  might expect.  (Starting an edit and exiting repositions the box
  above the baseline, so it does look like a layout bug.)  

  Workaround is to click in the title, then use tab to shift focus to
  the location.  (Or use double click to open the edit dialog.)

* If a field is not edited, it is possible to tab or shift-tab to
  shift focus to next field, but after editing a field, tabbing to
  the next field saves the editted field, which currently causes the
  whole itembox to be deleted and redisplayed.

* On a weekview with 160 15-minute events, redisplay is near 50% slower
  than before this patch.  (See ideas for future below).

Refactoring:

Introduced calendar-item-field-textbox used for each field.
calendar-item-box
  contains the title field textbox.
calendar-month-day-box-item extends calendar-item-box.
  Contains a label for the time and the title field textbox.
calendar-item-timespan-box extends calendar-item-box.
  Contains gripbars and the title, location, and description field texboxes.

Introduced method initializeFromOccurrence so derived bindings can
  extend this method and call the base method to avoid duplication.
  calendar-month-day-box-item extends the method to initialize the
    time label.
  calendar-item-timespan-box extends this method to initialize the
    location and description fields
  (Previously, the derived bindings duplicated the occurrence
  property, copying all the the setter code in the base binding 
  in order to add to it.  Now the setter calls initializeFromOccurrence
  after setting the occurrence.)

Click handling is now split between the
  calendar-item-box, which handles selection and tabbing focus, and
  calendar-item-field-textbox, which handles start/end editing a field.
  Custom FieldEditStarted events and FieldEditStopped events are used to
    notify parents when to start and end edit mode (to ignore clicks).

Simplifying:

calendar-item-timespan-gripbar parentorient attribute, and several
  listeners and methods to keep it up to date, were all removed.
  (It was used only for the CSS cursor, but changing to use
   a CSS rule referencing calendar-item-timespan[orient="..."]
   seems to be just as fast and simplifies the code.)

calendar-month-view.relayout
  removed code that initialized item-box "class" and "tooltip" since that
  duplicates the previous initialization in the item-box constructor.

Renaming:

Renamed calendar-editable-item to calendar-item-box
  (editable parts moved to calendar-item-field-textbox)
Renamed calendar-event-box to calendar-item-timespan-box, 
and calendar-event-gripbar to calendar-item-timespan-gripbar
  to make clearer it is not used in month-view, and that
  it may display items other than events (such as tasks).
  (It was confusing to see closely related bindings named
   with '-item' in month-view and view-core, but '-event' in
   multiday-view.)

Ideas for future:
* speed: don't create editing textbox until it is needed.
* speed: test whether userchrome can override xbl bindings; 
  if so, then create separate versions of multiday-view which 
  creating different day-view and week-view items.
* field: Attendees (some meetings are better identified by attendee rather 
  than title, e.g., tutor/consultant/doctor/dentist/lawyer/hair appts.).
Attachment #248250 - Flags: first-review?(lilmatt)
While I'm impressed with the refactoring, I'm not really comfortable taking it in 0.5 with so many of those minor issues outstanding.

Perhaps you can continue to refine this to address as many of them as you can?  I also suggest keeping up with mickey's work in bug 368982 so you guys don't duplicate effort (for this bug and your shrinkable/expandable view bug)

Removing review request for now
Attachment #248250 - Flags: first-review?(lilmatt)
Duplicate of this bug: 375637
Duplicate of this bug: 401359
how is the actual state of this "bug"?

it has been reported three years ago and I think it isn't so hard do implement.

I would be very proud if you could implement it for the next release.

THANKS
Duplicate of this bug: 441540
Snapshot of schedule of a medium size multi-track symposium/summit (3day, up to 7 parallel sessions: test narrow events).
- Titles, locations, descriptions, and URIs are included (test wrapping and pre-formatted descriptions).  Titles include presenters where available.
- Category is same as location room, so by coloring categories user can see tracks.  (test category marking)
- Dates are 'local/floating' (no timezone) so that it can be used by testers in any timezone (schedule won't break over midnight in any tester timezone)
- a few are marked tentative, confirmed, or cancelled (test status marking)
Attachment #207047 - Attachment is obsolete: true
3-day work-week only is shown.

Category colors used in image to show tracks (category is room name):
  Emerald:    turquoise
  Callaghan:  dark green
  Crystal:    light yellow
  Alpine C:   tan
  Alpine D:   light brown
  Alpine E:   dark brown
  Alta:       black
  Glacier:    blue

italic is tentative
bold is confirmed
line-thru is cancelled
Attachment #207048 - Attachment is obsolete: true
140 events in one work week

Goal: Advancing to this week from previous week should be about the same speed, not slower (previous patch was slower, took 1.5 times the time).
Attachment #248250 - Attachment description: v2: v1. add location and description to multiday-view item box (refactor item-boxes) → v2 patch: add location and description to multiday-view item box (refactor item-boxes)
Updated patch from comment 17.

New performance change:  textbox for each text field is not created unless needed, so this version is about the same speed as the current version (unlike previous patch), as tested using the performance test data (comment 26).

New refactoring:  calendar-item-frame implements the common elements from month-view-day-box-item, multiday-view calendar-item-allday-box, and multiday-view calendar-item-timespan-box.  These include the border, the background color, the selection color, the background gradient, and the category stripe.
Attachment #248250 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Gekacheka, is your latest patch ready for review or are you still working on it?
How soon is 0.9, and is this too risky for 0.9? (or too risky to add after 0.9 with 1.0 next?)  

My initial thought was to wait until after 0.9 since 0.9 is behind schedule and this hasn't been flagged on the schedule for 0.9, but I can see maybe it is too big a UI change to introduce in 1.0 without wider than nightly-user testing.  Ideally it would have both a period of nightly-user testing and some-release-before-1.0 user testing.  

(BTW, the patch will no longer apply after some patches for bugs which *are* flagged for 0.9, like bug 437412, so I expect at least one update will be needed.)
Status: ASSIGNED → NEW
Component: Internal Components → Calendar Views
Flags: wanted-calendar0.9?
QA Contact: base → views
Status: NEW → ASSIGNED
I think we should refrain from further UI changes for 0.9 and concentrate on getting the blocking bugs fixed. This is an excellent candidate to land shortly after 0.9 though.
Flags: wanted-calendar0.9? → wanted-calendar0.9-
Suggestion:

Allow for custom formatting of fields.  %sd = start date, %t = title, etc.  Formatting can be applied to single instances, reoccurring events, calendars, different views, etc.
Summary: Location and description do not appear in multiday view event box → Location does not appear in multiday view event box
Duplicate of this bug: 394153
(In reply to Matthew Monaco from comment #32)
> Suggestion: Allow for custom formatting of fields. [...]

Agreed. It's very inconvenient having to work-around this bug by adding the event location to the title or even the calender name.
Hi Community,

This is my First comment, so please go easy on me.
I have included a location field in daily/weekly view:
<Event>
grippyTop
Title(Editable)
Location
empty Space
grippyBottom
</Event>

Added a Checkbox in Preferences Show Location in daily/weekly view.
(I have problems translating the Pref-String).

So far it works and looks good in 3.3.2 and 3.9.1 Lightning-Calendar

I still need a good example of how to translate strings for Preferences, if i just add pref.showlocation.label to dtd it crashes the Preferences View (general in my case). Don't know why :/
Since the translation is the last Problem, i will try to solve that and afterwards upload the files i worked, which are so far:
chrome/calendar/content/calendar/calendar-view-core.xml -> create fields and base layout (box) + property & method

chrome/calendar/content/calendar/calendar-multiday-view.xml -> create fields and base layout (box)

chrome/calendar/content/calendar/calendar-base-view.xml -> observer for Preference

chrome/calendar/content/calendar/preferences/general.xul -> create Preference

chrome/skin/common/calendar-views.css -> Style
Hi Clemens,

if with crashing the preferences view you mean the yellow DTD error thing, then its probably because the dtd you added the string in is not part of that dialog or window. You need to make sure you add the dtd entry to a file that is included. Check other strings around it and figure out in which file they are.

This file is a good candidate if your string is in the "views" tab of the preferences.

http://mxr.mozilla.org/comm-central/source/calendar/locales/en-US/chrome/calendar/preferences/views.dtd


When you upload the file changes, can you upload them as a patch against http://hg.mozilla.org/comm-central/ ? Also, for review a screenshot would be nice.
Posted file calendar-view-core.xml (obsolete) β€”
hopefully the diff comes automatically
Attachment #8565962 - Attachment is obsolete: true
Unfortunately the diffs aren't created automatically. If you need help with that please let me know, there is a guide available here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
Posted patch location_v1.patch (obsolete) β€” β€” Splinter Review
Hello again,

i finally could upload a patch, that is diffed to the actual version (lightning 4.0a1). Check it out, I will add a screenshot too.

Feedback appreciated.
Posted image Screenshot of location_v1.patch in action (obsolete) β€”
(In reply to Clemens from comment #39)
> [...] Feedback appreciated.

It looks very neat and IMHO completely solves the problem :-)
Just 1 small question: In even smaller event boxes, will the location take precedence over the title? IMHO, both should be abbreviated if necessary, rather than not showing any location info at all.
Attachment #8567969 - Flags: review?(philipp)
Assignee: gekacheka → clemens
Comment on attachment 8567969 [details] [diff] [review]
location_v1.patch

Review of attachment 8567969 [details] [diff] [review]:
-----------------------------------------------------------------

A first pass at the review, I have yet to test it. Most are just minor nits, but I'd appreciate if you could upload a new patch with the comments fixed.

::: calendar/base/content/calendar-base-view.xml
@@ +644,5 @@
>                 case "calendar.date.format":
>                     this.refreshView();
>                     break;
> +               case "calendar.view.showLocation":
> +                   this.refreshView();

Since its the same command, you can just have it fall through.

::: calendar/base/content/calendar-multiday-view.xml
@@ +2255,3 @@
>                            anonid="calendar-event-details"
>                            align="start">
>                    <xul:image anonid="item-icon"

This will make the image be above location and summary, wasting space for short one-line events.

@@ +2396,5 @@
> +          var width = document.getAnonymousElementByAttribute(this, "anonid", "eventbox").boxObject.width;
> +          evl.setAttribute("style", "width: " + width + "px;max-height: " + Math.max(0, height-gripbar*2) + "px");
> +          //evl.setAttribute("style", "width: " + width + "px;height:20px");
> +          evloc.setAttribute("style", "width: " + width + "px;max-height: " + Math.max(0, height-gripbar*2) + "px");
> +          //evloc.setAttribute("style", "width: " + width + "px;height:20px");

Please remove the commented out lines.

::: calendar/base/content/calendar-view-core.xml
@@ +42,5 @@
>                                   wrap="true"/>
> +                    <xul:label anonid="event-location"
> +                               crop="end"
> +                               flex="1"
> +                               style="margin: 0;white-space: pre;"

The style for this should likely go into a css class instead. Also, I'm not sure its intended, but since you have flex=1 on both the label and the spacer below, they will take an equal amount of space. You might just want to remove the spacer.

@@ +141,4 @@
>          <setter><![CDATA[
>            this.mOccurrence = val;
>            this.setEditableLabel();
> +          this.setLocationLabel(); //CN - Location - Gorilla Mod

Go ahead and remove the comment here :)

@@ +163,5 @@
>          ]]></body>
>        </method>
> +      <method name="setLocationLabel">
> +      <body><![CDATA[
> +        var evloc = document.getAnonymousElementByAttribute(this, "anonid", "event-location");

Can you use this.eventLocationLabel here?

@@ +170,5 @@
> +        if (showLoc  && (item.getProperty("LOCATION") != null) ){
> +        evloc.textContent = item.getProperty("LOCATION");
> +        }else{
> +        this.eventLocationLabel.setAttribute("hidden", "true" );
> +        }

Could you check the indents and spacing in this function? Spaces instead of tabs, space before/after { and }, space after comma, no space before ); etc.

@@ +272,5 @@
>            this.eventNameLabel.setAttribute("hidden", "true");
>  
>            this.mEditing = true;
> +          var width = document.getAnonymousElementByAttribute(this, "anonid", "eventbox").boxObject.width;
> +          this.eventNameTextbox.setAttribute("style", "width: " + width + "px;");

Can't the width be determined automatically, e.g. with the flex attribute or similar?

::: calendar/base/content/preferences/general.xul
@@ +90,5 @@
>                  </menulist>
> +              </hbox>
> +                <checkbox id="showLocation" pack="end"
> +                          label="&pref.showlocation.label;"
> +                          preference="calendar.view.showLocation"/>

Please check indent here.

::: calendar/base/themes/common/calendar-views.css
@@ +508,5 @@
> +    width: auto;
> +    margin: 0px;    
> +    font-weight: normal;
> +    opacity:0.5;
> +}

You could split this up into two rules, one for .calendar-event-details-core and the other only containing the opacity with .calendar-event-details-core.location-desc.

Also, some trailing whitespaces snuck in here.

::: calendar/locales/en-US/chrome/calendar/preferences/general.dtd
@@ +40,4 @@
>  <!ENTITY pref.accessibility.label "Accessibility" >
>  <!ENTITY pref.systemcolors.label "Optimize colors for accessibility" >
>  <!ENTITY pref.systemcolors.accesskey "c">
> +<!ENTITY pref.showlocation.label "Show Location below Title" >

Please change casing to "Show location below title". I'm never sure how its correct in English, but unless the string is a title itself, most words should be lowercase.
Comment on attachment 8567969 [details] [diff] [review]
location_v1.patch

Issues noticed during testing:


1) Create an event in week view via drag
Result: The summary shows up in the middle of the event, instead of at the top
Note: Same thing happens when using click-to-edit

2) I don't know if its just me, but for certain calendar colors (like #7F3A69), when the title text is white (and readable), the location text is not-so-readable. I didn't re-read if it was mentioned before, but could you make sure that at least in system colors mode the normal text color is used, maybe with a slightly increased margin in that case?

3) Probably also discussed in the last comments (didn't read the bug and my battery is about to die), but the preference normally fits better into the "Views" tab, which is already pretty full. Maybe we can cram it on the right side of the "start the week on" option?


r- to get a new patch and fix these two issues, but I think this is a great step forward.
Attachment #8567969 - Flags: review?(philipp) → review-
The bug looks almost solved but the last comment is over a year old and there's no release targeted. What about this bug? Can we still hope?
Posted patch location_v2.patch (obsolete) β€” β€” Splinter Review
Feedback appreciated, everything works except shorten Text with ... via CSS
Attachment #8815850 - Flags: ui-review+
Attachment #8815850 - Flags: review+
Attachment #8815850 - Flags: feedback+
Attachment #8815850 - Flags: ui-review?
Attachment #8815850 - Flags: ui-review+
Attachment #8815850 - Flags: review?(philipp)
Attachment #8815850 - Flags: review+
Attachment #8815850 - Flags: feedback?
Attachment #8815850 - Flags: feedback+
Is been a long time. I went through your list philipp. If you are still alive you can rereview now. and Bring it to it's final place. 
The only thing I didn't find out is how to make the Text (Title, Location) be shortened with CSS text-over-flow.
Currently rebuilding but since these are rather minor changes I do not expect problems.
Cheers
Thanks for coming back! I'll take a look at this soon, but may be a while since I'll be at an event next week. You can use crop="end" to make the ... appear. https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/label#a-crop
I tried to use crop, but it doesn't work. I get line breaks if there is a space in the title or locationname but I don't get a crop like in monthview for example.
<xul:label anonid="event-location" class="calendar-event-details-core location-desc" crop="end"/> doesn't work but I see that there are linebreaks so there is a width. Have not figured that out yet.
As mentioned this does not work for the title as well.
Hi Philipp, you think we can ship this patch out like that or does it something more?
Kind regards
Comment on attachment 8815850 [details] [diff] [review]
location_v2.patch

Richard, can you help out Clemens on comment 48?
Flags: needinfo?(richard.marti)
Attachment #8815850 - Flags: ui-review?
Attachment #8815850 - Flags: feedback?
(In reply to [:MakeMyDay] from comment #50)
> Comment on attachment 8815850 [details] [diff] [review]
> location_v2.patch
> 
> Richard, can you help out Clemens on comment 48?

With adding crop="end" at <xul:label anonid="event-location" class="calendar-event-details-core location-desc"/> in calendar-multiday-view.xml
and changing in calendar-view-core.xml
evloc.textContent = item.getProperty("LOCATION");
to
evloc.value = item.getProperty("LOCATION");

gives the ellipsis. Doing the same for the title should also add the ellipsis there.
Flags: needinfo?(richard.marti)
Clemens, can you look into Richard's suggestion?
Flags: needinfo?(clemens)
I made the changes, it is working (partially) for the location Field and only if there not more Events in the same day and time. The title field is somewhat resistant to cropping maybe that's intended. Uploaded the new version.
Flags: needinfo?(clemens)
Posted patch location_v3.patch (obsolete) β€” β€” Splinter Review
Attachment #8567969 - Attachment is obsolete: true
Attachment #8906999 - Flags: review?(philipp)
Thank you :-)
Comment on attachment 8906999 [details] [diff] [review]
location_v3.patch

Review of attachment 8906999 [details] [diff] [review]:
-----------------------------------------------------------------

Codewise, some minor nits. Richard, I'd appreciate if you could take a look at the new patch and see how it feels. In the past the cropping in the views has always been a bit annoying, hope we can get it fixed now!

Would also be good to test this with a zero-length event or similar.

::: calendar/base/content/calendar-view-core.xml
@@ +161,5 @@
>        </method>
>  
> +      <method name="setLocationLabel">
> +        <body><![CDATA[
> +          var evloc = document.getAnonymousElementByAttribute(this, "anonid", "event-location");

let's use let instead of var for new functions like this one.

@@ +164,5 @@
> +        <body><![CDATA[
> +          var evloc = document.getAnonymousElementByAttribute(this, "anonid", "event-location");
> +          var item = this.mOccurrence;
> +          var showLoc = Preferences.get("calendar.view.showLocation",false);
> +          if (showLoc  && (item.getProperty("LOCATION") != null)) {

Can you check indent and spacing in this function? There should only be one space before and after operators like &&, the indent should use spaces and blocks should be indented.

@@ +170,5 @@
> +          } else {
> +          this.eventLocationLabel.setAttribute("hidden", "true" );
> +          }
> +        ]]></body>
> +      </method>      

Whitespaces at end of line here

::: calendar/base/content/preferences/views.xul
@@ +70,5 @@
>                          name="calendar.previousweeks.inview"
>                          type="int"/>
> +            <preference id="calendar.view.showLocation"
> +                        name="calendar.view.showLocation"
> +                        type="bool"/> 

Whitespace here too

::: calendar/base/themes/common/calendar-views.css
@@ +500,4 @@
>  }
>  
>  /** End time bar **/
> +.calendar-event-details-core.title-desc,.calendar-event-details-core.location-desc {

Can you put the selectors into multiple lines, or at least put a space after the comma?

::: calendar/locales/en-US/chrome/calendar/preferences/views.dtd
@@ +37,4 @@
>  <!ENTITY pref.numberofweeks.4 "4 weeks">
>  <!ENTITY pref.numberofweeks.5 "5 weeks">
>  <!ENTITY pref.numberofweeks.6 "6 weeks">
> +<!ENTITY pref.showlocation.label "Show location below title" >

I'd just keep this as "Show location" or "Show location in view", the positioning is not really relevant and a technical detail.
Attachment #8906999 - Flags: ui-review?(richard.marti)
Attachment #8906999 - Flags: review?(philipp)
Attachment #8906999 - Flags: review+
Comment on attachment 8906999 [details] [diff] [review]
location_v3.patch

The cropping works for the location when the title isn't too long. When in the title there is a word which is longer than the event container the longer text is no more visible and the location has the same width and thus the ellipsis is also no more visible.

This patch isn't the cause of this as it happens without it too. ui-r+

Thank you Clemens.
Attachment #8906999 - Flags: ui-review?(richard.marti) → ui-review+
Clemens, can you upload a new patch with nits fixed? Feel free to mark old patches as obsolete when uploading the new version.
Flags: needinfo?(clemens)
Posted patch locv4.patch (obsolete) β€” β€” Splinter Review
Hi Philipp,
took a while but I cleaned up the patch (whitespaces and indents). Maybe you can rethink the Text 'Show location' as '... with title' or so, because otherwise it might not be clear except for those who already wait 12 years for this feature :)
I hope it still fits the current build.
Cheers,
Clemens
Attachment #8567976 - Attachment is obsolete: true
Attachment #8815850 - Attachment is obsolete: true
Attachment #8906999 - Attachment is obsolete: true
Attachment #8815850 - Flags: review?(philipp)
Flags: needinfo?(clemens)
Attachment #8932462 - Flags: review?(philipp)
This patch has string changes (though not sure whether a hidden pref wouldn't be sufficient here), so we would need to get it in in the current cycle to be in the next release. In adding it to the tracking list to keep it on the radar.
Blocks: ltn62
Whiteboard: [has l10n impact]
Posted patch Fix - v5 β€” β€” Splinter Review
Here is an updated patch, also with prior review comments taken care of and a few nits fixed
Attachment #8932462 - Attachment is obsolete: true
Attachment #8932462 - Flags: review?(philipp)
Attachment #8958142 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c3605c05a4d4
Location does not appear in multiday view event box. r=philipp
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Need target 6.3.
Flags: needinfo?(philipp)
Target Milestone: --- → 6.2
Flags: needinfo?(philipp)
Target Milestone: 6.2 → 6.3
Attachment #8958142 - Flags: approval-calendar-beta+
Technically this was pushed with late-l10n, since I wasn't able to make it in time for the merge yesterday. Marking as such.
Keywords: late-l10n
Target Milestone: 6.3 → 6.2
Whoop, whoop :-) Thank you all so much!
You need to log in before you can comment on or make changes to this bug.