Page MenuHomePhorge
Differential D25788

Remove unused PHUICalendarMonthView::getMaxDailyEventsForWeek()
ClosedPublic
Actions

Authored by aklapper on Aug 16 2024, 07:18.
Tags
None
Referenced Files
F4011741: D25788.1746626567.diff
Tue, May 6, 14:02
F3878204: D25788.1746218974.diff
Thu, May 1, 20:49
F3873609: D25788.1746192084.diff
Thu, May 1, 13:21
F3833114: D25788.1746081132.diff
Wed, Apr 30, 06:32
F3605280: D25788.1745260375.diff
Sun, Apr 20, 18:32
F3605231: D25788.1745258556.diff
Sun, Apr 20, 18:02
F3553062: D25788.1744917074.diff
Wed, Apr 16, 19:11
F3553006: D25788.1744916003.diff
Wed, Apr 16, 18:53
View All 48 Files
Subscribers
Cigaryno
Matthew
tobiaswiese
valerio.bozzolan

Details

Reviewers
valerio.bozzolan
Group Reviewers
O1: Blessed Committers
Commits
rP66bd13b193b6: Remove unused PHUICalendarMonthView::getMaxDailyEventsForWeek()
Summary

Call to this private function was removed in https://we.phorge.it/rPaee9d88c17e25b924ecd5293691cc6fe38d6c7d9.

Test Plan

Grep the code; run static code analysis.

Diff Detail

Repository
rP Phorge
Branch
unusedMethods5
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1538
Build 1538: arc lint + arc unit

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)Packages
src/
view/
phui/
calendar/
12 lines0%
Loading...
O1: Blessed Committers

Diff 2383

View Options

src/view/phui/calendar/PHUICalendarMonthView.php

Show All 10 Linesfinal class PhabricatorMarkupOneOff
private $preserveLinebreaks; private $preserveLinebreaks;
private $engineRuleset; private $engineRuleset;
private $engine; private $engine;
private $disableCache; private $disableCache;
private $contentCacheFragment; private $contentCacheFragment;
private $generateTableOfContents; private $generateTableOfContents;
private $tableOfContents; private $tableOfContents;
private $collapsibleIndexInstance = 0;
public function setEngineRuleset($engine_ruleset) { public function setEngineRuleset($engine_ruleset) {
$this->engineRuleset = $engine_ruleset; $this->engineRuleset = $engine_ruleset;
return $this; return $this;
} }
public function getEngineRuleset() { public function getEngineRuleset() {
return $this->engineRuleset; return $this->engineRuleset;
▲ Show 20 Lines • Show All 65 Lines • ▼ Show 20 Linesfinal class PhabricatorMarkupOneOff
public function newMarkupEngine($field) { public function newMarkupEngine($field) {
if ($this->engine) { if ($this->engine) {
return $this->engine; return $this->engine;
} }
if ($this->engineRuleset) { if ($this->engineRuleset) {
return PhabricatorMarkupEngine::getEngine($this->engineRuleset); return PhabricatorMarkupEngine::getEngine($this->engineRuleset);
} else if ($this->preserveLinebreaks) { } else if ($this->preserveLinebreaks) {
return PhabricatorMarkupEngine::getEngine(); $engine = PhabricatorMarkupEngine::getEngine();
if ($this->generateTableOfContents) {
$engine->setConfig('header.generate-toc', true);
}
return $engine;
} else { } else {
return PhabricatorMarkupEngine::getEngine('nolinebreaks'); return PhabricatorMarkupEngine::getEngine('nolinebreaks');
} }
} }
public function getMarkupText($field) { public function getMarkupText($field) {
return $this->getContent(); return $this->getContent();
} }
public function didMarkupText( public function didMarkupText(
$field, $field,
$output, $output,
PhutilMarkupEngine $engine) { PhutilMarkupEngine $engine) {
if ($this->getGenerateTableOfContents()) { if ($this->getGenerateTableOfContents()) {
$toc = PhutilRemarkupHeaderBlockRule::renderTableOfContents($engine); $toc = PhutilRemarkupHeaderBlockRule::renderTableOfContents($engine);
$this->tableOfContents = $toc; $this->tableOfContents = $toc;
$collapsible_index = $this->buildCollapsibleIndex($toc);
valerio.bozzolanUnsubmitted
Done
Inline Actions

This may be already safe from an XSS perspective but, as far as I understand, this is the way we should adopt:

array($toc, $output)

Reason:

Combine elements with arrays, not string concatenation.

https://we.phorge.it/book/contrib/article/rendering_html/

valerio.bozzolan: This may be already safe from an XSS perspective but, as far as I understand, this is the way…
valerio.bozzolanUnsubmitted
Not Done
Inline Actions
$this->tableOfContents = $toc;
- $output = phutil_implode_html("\n", array($toc, $output));
+ $output = array($toc, $output);
}
require_celerity_resource('phabricator-remarkup-css');

Sorry my friend, I mean this, with just an array, as the documentation indicates under "Render a tag containing other tags safely"

valerio.bozzolan: Sorry my friend, I mean this, with just an array, as the documentation indicates under "Render…
StenAuthorUnsubmitted
Done
Inline Actions

Return an array when the function is expected to return a string-like object? I don't think that would be a good idea!
phutil_implode_html() is an excellent function though as it provides a guarantee that what it generates is safe HTML. Much better - no need for a maintainer to look any deeper for unsafe HTML.

Sten: Return an array when the function is expected to return a string-like object? I don't think…
valerio.bozzolanUnsubmitted
Not Done
Inline Actions

(This thread is a non-blocking suggestion)

Premising that, to me, your solution is nice and readable and I see the point.

We also should see that in this function we already always return a phutil_tag(), where its third argument accepts an array for exactly this case where we have multiple HTML things to be appended. We have not a super-big documentation, but that is included in there, so I think it would be OK to take advantage of that.

Again, let's see the official example:

https://we.phorge.it/book/contrib/article/rendering_html/
phutil_tag('div', array(), array($header, $body));

So the suggestion was just about that.

Incidentally, doing this, we avoid to send a newline. In 100,000 years, this could save a network cable.

End of the tip ❤

valerio.bozzolan: (This thread is a non-blocking suggestion) Premising that, to me, your solution is nice and…
StenAuthorUnsubmitted
Done
Inline Actions

Actually I like the newline - makes debugging a little clearer. For low volume traffic like this, maintainability trumps performance.

Also, the 'div' tag adds a minimum of 11 chars, and additional unwarranted complexity.

Sten: Actually I like the newline - makes debugging a little clearer. For low volume traffic like…
valerio.bozzolanUnsubmitted
Done
Inline Actions
$this->tableOfContents = $toc;
- $collapsible_button_id = 'index button 1';
+ $collapsible_button_id = 'toc-index-toggler-button';
$collapsible_div_id = 'index div 1';

It's quite unique to have HTML id="" with spaces. Maybe with dashes. Proposed something

valerio.bozzolan: It's quite unique to have HTML id="" with spaces. Maybe with dashes. Proposed something
$output = phutil_implode_html("\n", array($collapsible_index, $output));
} }
valerio.bozzolanUnsubmitted
Not Done
Inline Actions

(This is an old Revision that contains an example of addSigil() that is probably better than an ID on the label itself)

(Part 1)

valerio.bozzolan: (This is an old Revision that contains an example of `addSigil()` that is probably better than…
valerio.bozzolanUnsubmitted
Not Done
Inline Actions

I mean here there is the example with an icon () and the use of addSigil()

https://we.phorge.it/D25457?id=1516

I can also re-integrate if you like that icon

valerio.bozzolan: I mean here there is the example with an icon ({icon list}) and the use of `addSigil()` https…
require_celerity_resource('phabricator-remarkup-css'); require_celerity_resource('phabricator-remarkup-css');
return phutil_tag( return phutil_tag(
'div', 'div',
array( array(
'class' => 'phabricator-remarkup', 'class' => 'phabricator-remarkup',
), ),
$output); $output);
} }
/**
* Build a collapsible table of contents display consisting of a
* screen-wide button to toggle the display, and the table of
* contents itself.
* @param PhutilSafeHTML $tocThe table of contents HTML.
valerio.bozzolanUnsubmitted
Not Done
Inline Actions
* contents itself.
- * @param PhutilSafeHTML $tocThe table of contents HTML.
+ * @param PhutilSafeHTML $toc The table of contents HTML.
* @return PhutilSafeHTML Div containing a button and the toc.

Just a cute space here

valerio.bozzolan: Just a cute space here
valerio.bozzolanUnsubmitted
Not Done
Inline Actions

Just a cute space here

valerio.bozzolan: Just a cute space here
* @return PhutilSafeHTML Div containing a button and the toc.
*/
private function buildCollapsibleIndex($toc) {
++$this->collapsibleIndexInstance;
$instance = $this->collapsibleIndexInstance;
valerio.bozzolanUnsubmitted
Done
Inline Actions

I very love this thing, thanks

  1. I just suggest container_id since the id can be set to whatever element in theory, not just divs.
valerio.bozzolan: I very love this thing, thanks 1. I just suggest `container_id` since the `id` can be set to…
$config = array(
'container_id' => "toc-index-div-$instance",
'toggle_button_id' => "toc-index-button-$instance",
valerio.bozzolanUnsubmitted
Done
Inline Actions

Non-blocking tip:

I'm quite sure that "butt" in English means culo in Italian. Maybe just "open" and "close". Or "btn_open" and "btn_close".

valerio.bozzolan: Non-blocking tip: I'm quite sure that "butt" in English means //culo// in Italian. Maybe just…
'button_icon' => array(
'id' => "toc-index-button-icon-$instance",
'open_class' => 'fa-folder',
'close_class' => 'fa-folder-open',
),
'button_i18n' => array(
'id' => "toc-index-button-text-$instance",
'open_text' => pht('Open Index'),
'close_text' => pht('Close Index'),
),
);
$button_icon = id(new PHUIIconView())
->setID($config['button_icon']['id'])
->setIcon($config['button_icon']['open_class']);
$button_text_span = phutil_tag(
'span',
array('class' => 'collapsible', 'id' => $config['button_i18n']['id']),
$config['button_i18n']['open_text']);
$button = phutil_tag(
'button',
array(
'type' => 'button',
'class' => 'collapsible',
'id' => $config['toggle_button_id'],
),
phutil_implode_html("\n", array($button_icon, $button_text_span)));
$toc_div = phutil_tag(
'div',
array('class' => 'collapsible-content', 'id' => $config['container_id']),
$toc);
require_celerity_resource('font-fontawesome');
Javelin::initBehavior('collapsiblejx', $config);
return phutil_implode_html("\n", array($button, $toc_div));
}
public function shouldUseMarkupCache($field) { public function shouldUseMarkupCache($field) {
if ($this->getDisableCache()) { if ($this->getDisableCache()) {
return false; return false;
} }
return true; return true;
} }
} }
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0