Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion - calendar module (not sure how to do it) #3110

Closed
BKeyport opened this issue May 23, 2023 · 17 comments
Closed

Suggestion - calendar module (not sure how to do it) #3110

BKeyport opened this issue May 23, 2023 · 17 comments

Comments

@BKeyport
Copy link
Contributor

BKeyport commented May 23, 2023

I'd like to make a small change to the next release, but I have no clue on how to do it.

Currently, calendar logs by default "Broadcasting: X events."
I'd like to change it to be "Broadcasting: X events from Y"
X: Number of events
Y: Name of calendar.

This would make debugging easier without enabling the debug flag, which massively overdisplays what I'm looking for.

Thanks!

@rejas
Copy link
Collaborator

rejas commented May 23, 2023

With "Name of calendar" do you mean the url? That would be fairly easy to implement.

@BKeyport
Copy link
Contributor Author

Not exactly.

{
 url: "https://calendar.google.com/calendar/ical/en.usa%23holiday%40group.v.calendar.google.com/public/basic.ics",
 name: "Holdays",
},

The "name" here if used, if not, then the URL.

@sdetweil
Copy link
Collaborator

we don't have a name attribute. that is added into config for calendarExt3 and agenda.

you can see the whole config, so given one known value, you can find the rest.

@sdetweil
Copy link
Collaborator

but. we already have that function

from the doc
Screenshot_20230523_071236_Chrome

@BKeyport
Copy link
Contributor Author

sam: This is making logging better - not broadcast purposes. I'm aware It's already there - I already use the name as you can see from my code snip above.

@sdetweil
Copy link
Collaborator

sdetweil commented May 23, 2023

sorry, logging... where?

@BKeyport
Copy link
Contributor Author

BKeyport commented May 24, 2023

	this.broadcastEvents = function () {
		Log.info(`Calendar-Fetcher: Broadcasting ${events.length} events.`);
		eventsReceivedCallback(this);
	};

this statement in calendarfetcher.js

@rejas
Copy link
Collaborator

rejas commented May 24, 2023

We dont have much meta-information there about the calendar so passing name down there involves a lot of code just for logging purpose. Wouldnt the url be sufficient?

@BKeyport
Copy link
Contributor Author

The URL would be a fine backup. The name I'm looking for is in the config.js and passed through to the CALENDAR_EVENTS broadcast array in every event as a named item "calendarName", so I know it's stored somewhere in the code, I just can't find it.

@sdetweil
Copy link
Collaborator

the problem is things are all over the place.

calendar.js does and add calendar() which doesn't pass the name
this goes down to the node helper, which doesn't have the name, which starts an async fetcher, which doesn't get the name..

it gets the events back from the util lib, and sends them up to the socketNotificationReceived in calendar.js, but there is no name in the data.
so all we know is there are a bunch of events to broadcast.

I was about to post the 'simple' code yesterday, but it turned out to need changes in 10 places to pass the name around, just to put it in that message IF it was specified (95% of the time not)

@rejas
Copy link
Collaborator

rejas commented May 25, 2023

I was about to post the 'simple' code yesterday, but it turned out to need changes in 10 places to pass the name around, just to put it in that message IF it was specified (95% of the time not)

Yeah, saw that too so thats why I propose to jsut add the url to the output.

@sdetweil
Copy link
Collaborator

but the url contains private info.. and is quite long...

@BKeyport
Copy link
Contributor Author

Sam, compared to the raw data put out in debug mode, it's nothing, and it's being logged - if you have access to the log, you also have access to the config.js, no?

Silly questions, for my own benefit and clarification -

the function that creates this log item is doing the actual broadcast, right? Why can't we pull the name out of the broadcast string (calendarName), or is the spaghetti code too much for that, even? are the logging points in the wrong spot?

@sdetweil
Copy link
Collaborator

sdetweil commented May 25, 2023

the function that creates this log item is doing the actual broadcast, right?

no.. it puts out the message(in calendarfetcher.js, which doesn't know the config.js and doesn't have the name) ,

	this.broadcastEvents = function () {
		Log.info(`Calendar-Fetcher: Broadcasting ${events.length} events.`);
		eventsReceivedCallback(this);
	};

and then another function (in node_helper.js, which doesn't know the name) sends the events UP

broadcastEvents: function (fetcher, identifier) {
		this.sendSocketNotification("CALENDAR_EVENTS", {
			id: identifier,
			url: fetcher.url(),
			events: fetcher.events()
		});
	}

to the calendar.js (only thing that can actually broadcast)

if (notification === "CALENDAR_EVENTS") {
			if (this.hasCalendarURL(payload.url)) {
				this.calendarData[payload.url] = payload.events;
				this.error = null;
				this.loaded = true;

				if (this.config.broadcastEvents) {
					this.broadcastEvents();
				}
			}

and IT creates the message that is broadcast
but the message is long since printed

broadcastEvents: function () {
		const eventList = this.createEventList(false);
		for (const event of eventList) {
			event.symbol = this.symbolsForEvent(event);
			event.calendarName = this.calendarNameForUrl(event.url);
			event.color = this.colorForUrl(event.url, false);
			delete event.url;
		}

		this.sendNotification("CALENDAR_EVENTS", eventList);
	}

MAYBE the 'fix' is to move the message to here, at the actual broadcast
then we could use the function to get the name using the url

	calendarNameForUrl: function (url) {
		return this.getCalendarProperty(url, "name", "");
	},

@BKeyport
Copy link
Contributor Author

Thanks for the explaination.. I guess the short term solution then would be the URL, with some code streamlining in the future?

@sdetweil
Copy link
Collaborator

sdetweil commented Jun 1, 2023

so, I tried to implement moving the logging message as proposed above... oops.. calendar.js runs in the browser, SO the message comes out in the developers window, NOT in the terminal window like currently...

@rejas
Copy link
Collaborator

rejas commented Jun 6, 2023

Thanks for the explaination.. I guess the short term solution then would be the URL, with some code streamlining in the future?

Yes indeed. Do you want to tackle it?

rejas pushed a commit that referenced this issue Sep 27, 2023
MichMich added a commit that referenced this issue Oct 1, 2023
## [2.25.0] - 2023-10-01

Thanks to: @bugsounet, @dgoth, @dependabot, @kenzal, @Knapoc,
@KristjanESPERANTO, @martingron, @NolanKingdon, @Paranoid93,
@TeddyStarinvest and @Ybbet.

Special thanks to @khassel, @rejas and @sdetweil for taking over most
(if not all) of the work on this release as project collaborators. This
version would not be there without their effort. Thank you guys! You are
awesome!

> ⚠️ This release needs nodejs version >= `v18`, older releases have
reached end of life and will not work!

### Added

- Added UV Index support to OpenWeatherMap
- Added 'hideDuplicates' flag to the calendar module
- Added `allowOverrideNotification` to weather module to enable sending
current weather objects with the `CURRENT_WEATHER_OVERRIDE` notification
to supplement/replace the current weather displayed
- Added optional AnimateCSS animate for `hide()`, `show()`,
`updateDom()`
- Added AnimateIn and animateOut in module config definition
- Apply AnimateIn rules on the first start
- Added automatic client page reload when server was restarted by
setting `reloadAfterServerRestart: true` in `config.js`, per default
`false` (#3105)
- Added eventClass option for customEvents on the default calendar
- Added AnimateCSS integration in tests suite (#3206)
- Added npm dependabot [Reserved to developer] (#3210)
- Added improved logging for calendar (#3110)

### Removed

- **Breaking Change**: Removed `digest` authentication method from
calendar module (which was already broken since release `2.15.0`)

### Updated

- Update roboto fonts to version v5
- Update issue template
- Update dev/dependencies incl. electron to v26
- Replace pretty-quick by lint-staged
(<prettier/pretty-quick#164>)
- Update engine node >=18. v16 reached it's end of life. (#3170)
- Update typescript definition for modules
- Cleaned up nunjuck templates
- Replace `node-fetch` with internal fetch (#2649) and remove
`digest-fetch`
- Update the French translation according to the English file.
- Update dependabot incl. vendor/fonts (monthly check)
- Renew `package-lock.json` for release

### Fixed

- Fix engine check on npm install (#3135)
- Fix undefined formatTime method in clock module (#3143)
- Fix clientonly startup fails after async added (#3151)
- Fix electron width/heigth when using xrandr under bullseye
- Fix time issue with certain recurring events in calendar module
- Fix ipWhiteList test (#3179)
- Fix newsfeed: Convert HTML entities, codes and tag in description
(#3191)
- Respect width/height (no fullscreen) if set in electronOptions
(together with `fullscreen: false`) in `config.js` (#3174)
- Fix: AnimateCSS merge hide() and show() animated css class when we do
multiple call
- Fix `Uncaught SyntaxError: Identifier 'getCorsUrl' has already been
declared (at utils.js:1:1)` when using `clock` and `weather` module
(#3204)
- Fix overriding `config.js` when running tests (#3201)
- Fix issue in weathergov provider with probability of precipitation not
showing up on hourly or daily forecast

---------

Signed-off-by: naveen <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Karsten Hassel <[email protected]>
Co-authored-by: Malte Hallström <[email protected]>
Co-authored-by: Veeck <[email protected]>
Co-authored-by: veeck <[email protected]>
Co-authored-by: dWoolridge <[email protected]>
Co-authored-by: Johan <[email protected]>
Co-authored-by: Dario Mratovich <[email protected]>
Co-authored-by: Dario Mratovich <[email protected]>
Co-authored-by: Magnus <[email protected]>
Co-authored-by: Naveen <[email protected]>
Co-authored-by: buxxi <[email protected]>
Co-authored-by: Thomas Hirschberger <[email protected]>
Co-authored-by: Kristjan ESPERANTO <[email protected]>
Co-authored-by: Andrés Vanegas Jiménez <[email protected]>
Co-authored-by: Dave Child <[email protected]>
Co-authored-by: grenagit <[email protected]>
Co-authored-by: Grena <[email protected]>
Co-authored-by: Magnus Marthinsen <[email protected]>
Co-authored-by: Patrick <[email protected]>
Co-authored-by: Piotr Rajnisz <[email protected]>
Co-authored-by: Suthep Yonphimai <[email protected]>
Co-authored-by: CarJem Generations (Carter Wallace) <[email protected]>
Co-authored-by: Nicholas Fogal <[email protected]>
Co-authored-by: JakeBinney <[email protected]>
Co-authored-by: OWL4C <[email protected]>
Co-authored-by: Oscar Björkman <[email protected]>
Co-authored-by: Ismar Slomic <[email protected]>
Co-authored-by: Jørgen Veum-Wahlberg <[email protected]>
Co-authored-by: Eddie Hung <[email protected]>
Co-authored-by: Bugsounet - Cédric <[email protected]>
Co-authored-by: bugsounet <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Knapoc <[email protected]>
Co-authored-by: sam detweiler <[email protected]>
Co-authored-by: veeck <[email protected]>
Co-authored-by: Paranoid93 <[email protected]>
Co-authored-by: NolanKingdon <[email protected]>
Co-authored-by: J. Kenzal Hunter <[email protected]>
Co-authored-by: Teddy <[email protected]>
Co-authored-by: TeddyStarinvest <[email protected]>
Co-authored-by: martingron <[email protected]>
Co-authored-by: dgoth <[email protected]>
@khassel khassel closed this as completed Oct 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants