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 15 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
23 changes: 20 additions & 3 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 All @@ -31,6 +32,7 @@ class _SettingsPageState extends State<SettingsPage> {

// String describing the level of editing permissions that the user has.
String userPermissionString = '';
bool isSwitched = false;
Copy link
Member

Choose a reason for hiding this comment

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

?


@override
void initState() {
Expand Down Expand Up @@ -116,9 +118,7 @@ class _SettingsPageState extends State<SettingsPage> {
),
),
Visibility(
visible: authProvider.isAuthenticated &&
!authProvider.isAnonymous &&
authProvider.currentUserFromCache.isAdmin == true,
visible: authProvider.currentUserFromCache.isAdmin == true,
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 like a broken merge, please revert the condition.

child: ListTile(
key: const Key('AdminPanel'),
onTap: () =>
Expand Down Expand Up @@ -149,6 +149,23 @@ class _SettingsPageState extends State<SettingsPage> {
subtitle: Text(S.current.infoExportToGoogleCalendar),
),
),
Column(
children: [
SwitchPreference(
'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.

'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:
'show number of week in the semester instead of the calendar year',
Copy link
Member

Choose a reason for hiding this comment

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

This should start with uppercase to match the other strings on the page

),
],
),
PreferenceTitle(S.current.labelFeedback),
ListTile(
key: const ValueKey('feedback_and_issues'),
Expand Down
123 changes: 123 additions & 0 deletions lib/pages/timetable/view/lead_header.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
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';

// ignore: implementation_imports
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;
Set<int> holidayWeeks = {};
Copy link
Member

Choose a reason for hiding this comment

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

Except for maybe calendars, all these class attributes seem unnecessary.


@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);

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: Text(
getWeekNumber(),
Copy link
Member

Choose a reason for hiding this comment

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

Move the setting check here (see my comment below, I'd like to move the logic of fetching the week to the calendar itself, and the calendar shouldn't know about settings).

style: textStyle,
textAlign: TextAlign.center,
),
),
),
);
}

String getWeekNumber() {
Copy link
Member

Choose a reason for hiding this comment

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

Move this to the calendar class directly, maybe make it a getter? Like String get academicWeek (it't not really just a number).

if (calendar != null) {
final List<int> nonHolidayWeeks = calendar.nonHolidayWeeks.toList();
final DateInterval winterSession = DateInterval(
calendar.exams.first.startDate, calendar.exams.first.endDate);
final DateInterval summerSession = DateInterval(
calendar.exams.last.startDate, calendar.exams.last.endDate);
Copy link
Member

Choose a reason for hiding this comment

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

You may think you know what the exam sessions are, but the abstraction is there for a reason. Remember that we also have restanțe & sesiunea de diplomă for instance, we don't want variables for every one of them, that's why we're using a list of exams. Instead, in the condition below, we can just do something like
calendar.exams.reduce((value, element) => value || element.contains(widget.date)).

for (var i = 1; i < 53; i++) {
if (!nonHolidayWeeks.contains(i)) holidayWeeks.add(i);
}
final week =
((widget.date.dayOfYear - widget.date.dayOfWeek.value + 10) / 7)
.floor();
if (LeadHeader.academicWeekNumber == false) {
return week.toString();
} else {
if (!nonHolidayWeeks.contains(week)) {
if (winterSession.contains(widget.date) ||
summerSession.contains(widget.date)) {
return 'S';
} else {
return 'H';
}
} else {
return (nonHolidayWeeks.indexOf(week) + 1).toString();
}
}
} else {
return '0';
Copy link
Member

Choose a reason for hiding this comment

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

Rather than 0, let's just return an empty string.

}
}

@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;
calendar = calendars.values.last;
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work, what if we scroll to the previous year/semester?

We have some existing logic for this in the calendar selection dropdown when adding an event:
https://github.com/student-hub/acs-upb-mobile/blob/master/lib/pages/timetable/view/events/add_event_view.dart#L112-L131

Let's do the following:

  • add a int semesterForDate(DateTime date) method to AcademicCalendar that does something like this:
for (final semester in semesters) {
  if (date.isBeforeOrDuring(semester)) {
    // semester.id is represented as "semesterN", where "semester0" is the first semester
    return 1 + int.tryParse(semester.id[semester.id.length - 1]);
  }
}
return -1;
  • the code in add_event_view.dart can be simplified to:
final LocalDate date = widget.initialEvent.start.calendarDate ?? LocalDate.today();
calendars.forEach((c) {
  final semester = c.semesterForDate(date);
  if (semester != -1) {
    selectedCalendar = c;
    selectedSemester = semester;
    return;
  }
});
  • in this file, to fetch the right semester we can just do:
    calendars.where((c) => c.semesterForDate(date) != -1).

(Keep in mind I didn't actually compile this code, so try to understand the logic and make changes where necessary)

});
});
}
}
2 changes: 2 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,7 @@ class _TimetablePageState extends State<TimetablePage> {
child: Stack(
children: [
Timetable<UniEventInstance>(
leadingHeaderBuilder: (_, date) => LeadHeader(date),
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+37
version: 1.3.1+38

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