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

Andrei mirica19/replace year week number #297

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
5e85fb3
Add academic week number to the timetable.
AndreiMirica19 Oct 2, 2021
86ebbff
Add academic week number to the timetable.
AndreiMirica19 Oct 2, 2021
2bf465b
Add academic week number to the timetable.
AndreiMirica19 Oct 2, 2021
064d9f3
Add academic week number to the timetable.
AndreiMirica19 Oct 2, 2021
be030ab
Add academic week number to the timetable.
AndreiMirica19 Oct 2, 2021
7b5be1f
Add academic week number to the timetable.
AndreiMirica19 Oct 3, 2021
f33ccef
Add academic week number to the timetable.
AndreiMirica19 Oct 3, 2021
43c14bd
Add academic week number to the timetable.
AndreiMirica19 Oct 4, 2021
1d840f9
Merge branch 'master' into AndreiMirica19/Replace_year_week_number
AndreiMirica19 Oct 4, 2021
7e77048
Add academic week number to the timetable.
AndreiMirica19 Oct 4, 2021
b7a466d
Add academic week number to the timetable.
AndreiMirica19 Oct 4, 2021
a744eb4
Add academic week number to the timetable.
AndreiMirica19 Oct 4, 2021
e8f55f1
Add academic week number to the timetable.
AndreiMirica19 Oct 4, 2021
a63224d
Add academic week number to the timetable.
AndreiMirica19 Oct 7, 2021
15852c8
Add academic week number to the timetable.
AndreiMirica19 Oct 7, 2021
a56d7a7
Add academic week number to the timetable.
AndreiMirica19 Oct 8, 2021
c834c3d
Add academic week number to the timetable.
AndreiMirica19 Oct 12, 2021
5cfec27
Merge branch 'master' into AndreiMirica19/Replace_year_week_number
AndreiMirica19 Oct 12, 2021
bc1bf14
Add academic week number to the timetable.
AndreiMirica19 Oct 13, 2021
5f8c165
Merge remote-tracking branch 'origin/AndreiMirica19/Replace_year_week…
AndreiMirica19 Oct 13, 2021
c10f563
Add academic week number to the timetable.
AndreiMirica19 Oct 13, 2021
63d5f1e
Add academic week number to the timetable.
AndreiMirica19 Nov 1, 2021
7420458
Merge branch 'master' into AndreiMirica19/Replace_year_week_number
AndreiMirica19 Nov 1, 2021
5d18e0a
Add academic week number to the timetable.
AndreiMirica19 Nov 1, 2021
e5dcc6a
Merge remote-tracking branch 'origin/AndreiMirica19/Replace_year_week…
AndreiMirica19 Nov 1, 2021
41c9eca
Add academic week number to the timetable.
AndreiMirica19 Nov 6, 2021
79e8316
Add academic week number to the timetable.
AndreiMirica19 Nov 6, 2021
40ae3ef
Add academic week number to the timetable.
AndreiMirica19 Nov 10, 2021
f5e376c
Add academic week number to the timetable.
AndreiMirica19 Nov 10, 2021
2f3646d
Add academic week number to the timetable.
AndreiMirica19 Mar 14, 2022
e7b0e27
Merge branch 'master' into AndreiMirica19/Replace_year_week_number
AndreiMirica19 Mar 14, 2022
96bbac1
Add academic week number to the timetable.
AndreiMirica19 Mar 14, 2022
4235f8c
Merge remote-tracking branch 'origin/AndreiMirica19/Replace_year_week…
AndreiMirica19 Mar 14, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions android/fastlane/metadata/android/en-GB/changelogs/10041.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Added:
-Users can now choose from setting if they want to see in the week indicator,from the timetable,the academic week number.
Comment on lines +1 to +2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep in mind that this is addressed to users. Use the other changelogs as examples. This is as if I was talking to you and saying "Andrei can now pass the maths class". It's usually also good to keep it short, and be consistent with the previous formatting. Furthermore, with every release we need to include all the changes that happened since the last release (this you can usually see from the commit history).

Suggested change
Added:
-Users can now choose from setting if they want to see in the week indicator,from the timetable,the academic week number.
Added
- The option to show the number of the week in the semester instead of the week in the year.
- A feature to allow admins to send a confirmation email from the app when approving a permission request.
Fixed
- Some minor bugs and text issues.

Try to do the same thing in the other changelogs as well.

(and also, you'll need to rename them since the version was bumped since the last time you created these files)

2 changes: 2 additions & 0 deletions android/fastlane/metadata/android/en-US/changelogs/10041.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Added:
-Users can now choose from setting if they want to see in the week indicator,from the timetable,the academic week number.
2 changes: 2 additions & 0 deletions android/fastlane/metadata/android/ro/changelogs/10041.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Adăugat:
-Utilizatorii pot alege din setări dacă vor să vadă în timetable numărul săptămânii din calendarul academic.
2 changes: 2 additions & 0 deletions lib/generated/intl/messages_en.dart
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,8 @@ class MessageLookup extends MessageLookupByLibrary {
"settingsFeedbackForm" : MessageLookupByLibrary.simpleMessage("Contact us"),
"settingsItemAdmin" : MessageLookupByLibrary.simpleMessage("Admin Panel"),
"settingsItemDarkMode" : MessageLookupByLibrary.simpleMessage("Dark Mode"),
"settingsAcademicWeekNumber":MessageLookupByLibrary.simpleMessage("Academic week number"),
"settingsAcademicWeekNumberSubtitle":MessageLookupByLibrary.simpleMessage("Show number of week in the semester instead of the calendar year"),
"settingsItemEditingPermissions" : MessageLookupByLibrary.simpleMessage("Your editing permissions"),
"settingsItemLanguage" : MessageLookupByLibrary.simpleMessage("Language"),
"settingsItemLanguageAuto" : MessageLookupByLibrary.simpleMessage("Auto"),
Expand Down
2 changes: 2 additions & 0 deletions lib/generated/intl/messages_ro.dart
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,8 @@ class MessageLookup extends MessageLookupByLibrary {
"settingsFeedbackForm" : MessageLookupByLibrary.simpleMessage("Contactează-ne"),
"settingsItemAdmin" : MessageLookupByLibrary.simpleMessage("Panoul Administratorului"),
"settingsItemDarkMode" : MessageLookupByLibrary.simpleMessage("Mod Întunecat"),
"settingsAcademicWeekNumber":MessageLookupByLibrary.simpleMessage("Săptămâni academice"),
"settingsAcademicWeekNumberSubtitle":MessageLookupByLibrary.simpleMessage("Afișează săptămână din semestru în loc de cea din an"),
"settingsItemEditingPermissions" : MessageLookupByLibrary.simpleMessage("Permisiunile tale de editare"),
"settingsItemLanguage" : MessageLookupByLibrary.simpleMessage("Limbă"),
"settingsItemLanguageAuto" : MessageLookupByLibrary.simpleMessage("Auto"),
Expand Down
19 changes: 19 additions & 0 deletions lib/generated/l10n.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions lib/l10n/intl_en.arb
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,9 @@

"settingsTitlePersonalization": "Personalization",
"settingsItemDarkMode": "Dark Mode",
"settingsAcademicWeekNumber":"Academic week number",
"settingsTitleLocalization": "Localization",
"settingsAcademicWeekNumberSubtitle":"Show number of week in the semester instead of the calendar year"
"settingsTitleDataControl": "Data control",
"settingsItemEditingPermissions": "Your editing permissions",
"settingsPermissionsNone": "No special permissions",
Expand Down
2 changes: 2 additions & 0 deletions lib/l10n/intl_ro.arb
Original file line number Diff line number Diff line change
Expand Up @@ -215,9 +215,11 @@

"settingsTitlePersonalization": "Personalizare",
"settingsItemDarkMode": "Mod Întunecat",
"settingsAcademicWeekNumber":"Săptămâni academice",
"settingsTitleLocalization": "Localizare",
"settingsTitleDataControl": "Control date",
"settingsItemEditingPermissions": "Permisiunile tale de editare",
"settingsAcademicWeekNumberSubtitle":"Afișează săptămână din semestru în loc de cea din an"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cred că "săptămâna" sună mai bine decât "săptămână"

"settingsPermissionsNone": "Fără permisiuni speciale",
"settingsPermissionsAdd": "Permisiune pentru adăugare de date publice",
"settingsPermissionsEdit": "Permisiune pentru editare de date publice",
Expand Down
17 changes: 17 additions & 0 deletions lib/pages/settings/view/settings_page.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import 'package:acs_upb_mobile/generated/l10n.dart';
import 'package:acs_upb_mobile/navigation/routes.dart';
import 'package:acs_upb_mobile/pages/settings/service/request_provider.dart';
import 'package:acs_upb_mobile/pages/timetable/service/uni_event_provider.dart';
import 'package:acs_upb_mobile/pages/timetable/view/lead_header.dart';
import 'package:acs_upb_mobile/resources/locale_provider.dart';
import 'package:acs_upb_mobile/resources/utils.dart';
import 'package:acs_upb_mobile/widgets/icon_text.dart';
Expand Down Expand Up @@ -149,6 +150,22 @@ class _SettingsPageState extends State<SettingsPage> {
subtitle: Text(S.current.infoExportToGoogleCalendar),
),
),
Column(
children: [
SwitchPreference(
S.current.settingsAcademicWeekNumber,
'Academic week number',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a nicer key, like we did above (e.g. academic_week_number.

onEnable: () {
LeadHeader.academicWeekNumber = true;
},
onDisable: () {
LeadHeader.academicWeekNumber = false;
},
defaultVal: false,
desc: S.current.settingsAcademicWeekNumberSubtitle,
),
],
),
PreferenceTitle(S.current.labelFeedback),
ListTile(
key: const ValueKey('feedback_and_issues'),
Expand Down
42 changes: 42 additions & 0 deletions lib/pages/timetable/model/academic_calendar.dart
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import 'package:acs_upb_mobile/pages/timetable/model/events/all_day_event.dart';
import 'package:acs_upb_mobile/pages/timetable/view/lead_header.dart';
import 'package:acs_upb_mobile/resources/utils.dart';
import 'package:flutter/foundation.dart';
import 'package:time_machine/time_machine.dart';
Expand Down Expand Up @@ -77,4 +78,45 @@ class AcademicCalendar {

return weeksByYear.values.expand((e) => e).toSet();
}

int semesterForDate(LocalDate date) {
for (final semester in semesters) {
if (date._isBeforeOrDuring(semester)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the comment back, it was relevant.
// semester.id is represented as "semesterN", where "semester0" is the first semester

return 1 + int.tryParse(semester.id[semester.id.length - 1]);
}
}
return -1;
}

String getWeekNumber(LocalDate date) {
final week = ((date.dayOfYear - date.dayOfWeek.value + 10) / 7).floor();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this formula? It's not obvious at all what this does, add a comment to explain what's happening here (and maybe below too).

if (LeadHeader.academicWeekNumber == false) return week.toString();
Comment on lines +92 to +93
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AcademicCalendar shouldn't have to know anything about LeadHeader, plus static variables should generally be avoided. This check should be done inside LeadHeader.

You can have two separate methods in this class, e.g. getSemesterWeekNumber and getYearWeekNumber. But honestly, the second method has absolutely nothing to do with the academic calendar and could just be an extension method on LocalDate defined somewhere in LeadHeader or wherever it's needed.

https://en.wikipedia.org/wiki/Single-responsibility_principle

final int finalWeekOfFirstSem = ((semesters[0].endDate.dayOfYear -
semesters[0].endDate.dayOfWeek.value +
10) /
7)
.floor();
if (!nonHolidayWeeks.contains(week)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use shorten if

return 'H';
} else {
if (semesterForDate(date) == 1) {
return (nonHolidayWeeks.toList().indexOf(week) + 1).toString();
} else {
return ((nonHolidayWeeks.toList().indexOf(week) + 1) -
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this logic, you're kind of assuming that the holiday between the semesters is exactly one week. This is very likely true for ACS, but if we ever want to extend this, it will lead to a bug that will be tricky to figure out. Similarly, you're also assuming that we have only two semesters :) This is meant to be a generic API.

Instead of calculating the last week of the first semester, why not just get the first week of the each semester, find the index of that in nonHolidayWeeks and simply subtract that from .indexOf(week)? This would be a generic solution that works for any N semesters with arbitrary holidays.

(nonHolidayWeeks.toList().indexOf(finalWeekOfFirstSem) + 1))
.toString();
}
}
}
}

extension LocalDateComparisons on LocalDate {
bool _isDuring(AllDayUniEvent semester) {
return DateInterval(semester.startDate, semester.endDate).contains(this);
}

bool _isBeforeOrDuring(AllDayUniEvent semester) {
if (compareTo(semester.startDate) < 0) return true;
return _isDuring(semester);
}
}
26 changes: 11 additions & 15 deletions lib/pages/timetable/view/events/add_event_view.dart
Original file line number Diff line number Diff line change
Expand Up @@ -109,22 +109,18 @@ class _AddEventViewState extends State<AddEventView> {
? 2
: 1;
} else {
bool foundSemester = false;
for (final calendar in calendars.entries) {
for (final semester in calendar.value.semesters) {
final LocalDate date =
widget.initialEvent.start.calendarDate ?? LocalDate.today();
if (date.isBeforeOrDuring(semester)) {
// semester.id is represented as "semesterN", where "semester0" is the first semester
selectedSemester =
1 + int.tryParse(semester.id[semester.id.length - 1]);
selectedCalendar = calendar.key;
foundSemester = true;
break;
}
const bool foundSemester = false;
final LocalDate date =
widget.initialEvent.start.calendarDate ?? LocalDate.today();
calendars.forEach((key, value) {
final semester = value.semesterForDate(date);
if (semester != -1) {
selectedCalendar = key;
selectedSemester = semester;
return;
}
if (foundSemester) break;
}
});

if (!foundSemester) {
selectedCalendar = calendars.entries.last.value.id;
selectedSemester = 2;
Expand Down
92 changes: 92 additions & 0 deletions lib/pages/timetable/view/lead_header.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import 'package:acs_upb_mobile/authentication/model/user.dart';
import 'package:acs_upb_mobile/authentication/service/auth_provider.dart';
import 'package:acs_upb_mobile/pages/classes/model/class.dart';
import 'package:acs_upb_mobile/pages/classes/service/class_provider.dart';
import 'package:acs_upb_mobile/pages/people/model/person.dart';
import 'package:acs_upb_mobile/pages/people/service/person_provider.dart';
import 'package:acs_upb_mobile/pages/timetable/model/academic_calendar.dart';
import 'package:acs_upb_mobile/pages/timetable/service/uni_event_provider.dart';
import 'package:black_hole_flutter/black_hole_flutter.dart';
import 'package:flutter/material.dart';
import 'package:time_machine/time_machine.dart';

import 'package:timetable/src/theme.dart';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Don't import implementation files from another package


// ignore: implementation_imports
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment isn't necessary, this isn't an implementation import.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You removed the comment above on line 12, which was actually necessary instead of removing this comment. Please pay attention.

import 'package:provider/provider.dart';

class LeadHeader extends StatefulWidget {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename to TimetableLeadingHeader for a more descriptive name?

const LeadHeader(this.date, {Key key}) : super(key: key);
final LocalDate date;
static var academicWeekNumber = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static vars are a no-no, see my comment on academic_calendar.dart.


@override
_LeadHeaderState createState() => _LeadHeaderState();
}

class _LeadHeaderState extends State<LeadHeader> {
List<ClassHeader> classHeaders = [];
List<Person> classTeachers = [];
User user;
AcademicCalendar calendar;
Map<String, AcademicCalendar> calendars = {};
int academicWeek;

@override
Widget build(BuildContext context) {
final theme = context.theme;
final timetableTheme = context.timetableTheme;
final defaultBackgroundColor = theme.contrastColor.withOpacity(0.12);
final textStyle = timetableTheme?.weekIndicatorTextStyle ??
TextStyle(
color: defaultBackgroundColor
.alphaBlendOn(theme.scaffoldBackgroundColor)
.mediumEmphasisOnColor,
fontSize: 14);
final filteredCalendars = calendars?.values
?.where((calendar) => calendar.semesterForDate(widget.date) != -1);
final calendar = filteredCalendars.isEmpty ? null : filteredCalendars.first;
return Container(
child: Center(
child: Container(
width: 27,
height: 20,
Comment on lines +52 to +53
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be the reason why we're getting an overflow in the tests. We should generally avoid fixed sizes for things, since they'll look different depending on screen size (and may overflow on smaller screens).

Can't we use exactly Jonas' implementation and just replace the week.toString() part? I can tell that's what you started from and that's great, but I don't really understand all the changes.

decoration: BoxDecoration(
borderRadius: BorderRadius.circular(2),
color: defaultBackgroundColor,
border: Border.all(color: defaultBackgroundColor, width: 1)),
child: calendar == null
? Container()
: Text(
calendar.getWeekNumber(widget.date),
style: textStyle,
textAlign: TextAlign.center,
),
),
),
);
}

@override
void initState() {
if (!mounted) {
return;
}
Comment on lines +72 to +74
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks unnecessary.

super.initState();
user =
Provider.of<AuthProvider>(context, listen: false).currentUserFromCache;
Provider.of<ClassProvider>(context, listen: false)
.fetchClassHeaders(uid: user.uid)
.then((headers) => setState(() => classHeaders = headers));
Provider.of<PersonProvider>(context, listen: false)
.fetchPeople()
.then((teachers) => setState(() => classTeachers = teachers));
Comment on lines +76 to +83
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these necessary?

Provider.of<UniEventProvider>(context, listen: false)
.fetchCalendars()
.then((calendars) {
setState(() {
this.calendars = calendars;
});
});
}
}
3 changes: 3 additions & 0 deletions lib/pages/timetable/view/timetable_page.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import 'package:provider/provider.dart';
import 'package:recase/recase.dart';
import 'package:time_machine/time_machine.dart';
import 'package:timetable/timetable.dart';
import 'lead_header.dart';

class TimetablePage extends StatefulWidget {
const TimetablePage({Key key}) : super(key: key);
Expand Down Expand Up @@ -98,6 +99,8 @@ class _TimetablePageState extends State<TimetablePage> {
child: Stack(
children: [
Timetable<UniEventInstance>(
leadingHeaderBuilder: (_, date) =>
LeadHeader(date ?? LocalDate.today()),
controller: _controller,
dateHeaderBuilder: (_, date) => DateHeader(date),
eventBuilder: (event) => UniEventWidget(event),
Expand Down
2 changes: 1 addition & 1 deletion pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ description: A mobile application for students at ACS UPB.
# https://developer.apple.com/library/archive/documentation/General/Reference/InfoPlistKeyReference/Articles/CoreFoundationKeys.html
#
# ACS UPB Mobile uses semantic versioning. You can read more in the CONTRIBUTING.md file.
version: 1.3.1+41
version: 1.3.1+42

environment:
sdk: ">=2.7.0 <3.0.0"
Expand Down
Loading