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

A lot of changes: Kotlin, time/date formatting fixes, demo activity for whole view snapping... #97

Open
wants to merge 48 commits into
base: develop
Choose a base branch
from

Conversation

AndroidDeveloperLB
Copy link

@AndroidDeveloperLB AndroidDeveloperLB commented May 7, 2018

I've prepared a pull request that allows to snap by entire view (week by week, for example) based on this pull request, together with some fixes of issues I've found, and conversion to 100% Kotlin code.

Sadly, some issues that I've found still exist:

My repository is here:
https://github.com/AndroidDeveloperLB/Android-Week-View
You can use latest commit in the project, from here:
https://jitpack.io/#AndroidDeveloperLB/Android-Week-View

-added ability to scroll whole view (entire week, for example) like a ViewPager.
-made default first day of week to be the one of the calendar.
…es some delicate work.

-fixed some time/date formatting related issues:
alamkanak#497
alamkanak#495 (but not fixed RTL alignment issue)
-created a new activity to demonstrate the paging of entire view (example: week by week snapping), based on this pull request:
Quivr#88
@SkyleKayma
Copy link

I did not try your code actually. But there is some changes to do in the Kotlin code. I will try to test it this afternoon. I probably forgot to notify things in the code, so if someone has the courage to double check everything, that would be great.

@AndroidDeveloperLB
Copy link
Author

AndroidDeveloperLB commented May 7, 2018

@SkyleKayma I've now tested this fork:
https://github.com/openium/Android-Week-View
It has the snapping of whole view (week by week snapping, for example), without the issue I've written about here : #31 (comment)
But it has a serious issue when I switch to 3-days. Maybe it's fixated on 7 days...
Can you please check it out? Maybe it can help here to solve this issue?

About Kotlin, I think it can be shortened a lot, but as the code became more complex, and because I've done it multiple times already with the other forks (tried various ones till I got here), I decided to do less this time.

@SkyleKayma
Copy link

Can you explain to me what is the issue in 3days view with the changes you had with this PR ?
I tried the code given here, and all is working correctly.

You are on the real fork actually. It's the last one that is active. So if you need to do some changes with Kotlin code, it's clearly in this repo.

@AndroidDeveloperLB
Copy link
Author

@SkyleKayma My code doesn't have this issue. The 3-days view issue is of this repository that I've tested:
https://github.com/openium/Android-Week-View
The issue is that is snaps very bad in this case. I think it might try to snap every 7 days even if you are in this state.
I mentioned it because it doesn't have this issue that does exist in my code (and before I forked) :
#31 (comment)
If you know how to fix it, please let me know.

About the fork, I've now worked on even more changes and fixes.

One fix is of a bug I didn't even report yet, that the day-month formatting in the header doesn't consider the one of the device. In many countries, the day is before the month, so you should see something like "15/4" instead of "4/15" .
In the current implementation, it's always month and then day.

I really hope I can fix the other issues too.

@jhoobergs
Copy link
Member

@AndroidDeveloperLB First, thanks for your PR
It's hard to review such a large PR in which java is changed to kotlin etc.

I think I would merge this PR if you added tests (which we still don't have 😞 ) so we can be more certain about it functioning correct.

@SkyleKayma
Copy link

SkyleKayma commented May 7, 2018

I'm probably stupid (or I missed something), but why want to fix a bug that you do not have in your code? :/
This repo (https://github.com/openium/Android-Week-View) is in any case no longer maintained and has several merge that have never been added. (I will probably delete this one, if this PR is accepted)

Btw, your code is working well. If you could do the little changes I said before, it could be awesome. (Or tell me if you are not agree with what I wrote)

@AndroidDeveloperLB
Copy link
Author

AndroidDeveloperLB commented May 7, 2018

I don't want to fix a bug that I don't have in the code. I want to fix a bug that I do have in the code (and in the original code, before I even forked it).
It's just that the bug exists in my code, but not in his, so I thought maybe you could check it out and get a clue of how to fix it.
The other repository has a different bug, so I mentioned it too.

This bug is quite serious compared to the RTL and shortened text issues and easy to reproduce, so if you can fix it, please let me know.

About my code working well, I did it in a delicate way, after having experienced with the same procedure on other forks here (we want all code of ours to be in Kotlin, so I wanted to do it here too).
There will be a new pull request tomorrow with fewer changes:

  • The fix I've mentioned of day+month order
  • minor code cleaning
  • adding 2 new design-related (optional) features : write on the left of the week-days, have only the week-days to have a background (meaning the all-day events will have normal background).

@AndroidDeveloperLB
Copy link
Author

@jhoobergs I never have time for unit tests (literally) . Not on my free time and not at the office. We are just too few developers who are in tight schedule.

Quivr#98
-added a feature to put text next to the week days
-added a feature to set only the week days to have a background, instead of week-days and all-day events.
-replaced Java "List" class with "MutableList" instead of Kotlin "List", to avoid possible crashes at runtime (because Kotlin "List" cannot be modified and is more restricted). That being said, I think in most cases, it's ok to use Kotlin "List", because in many cases you don't really need to modify the lists.
-some code cleaning and refactoring, including conversion of some fields to be Kotlin properties (setters and getters near the field itself)
@AndroidDeveloperLB
Copy link
Author

AndroidDeveloperLB commented May 8, 2018

OK here I've made a new commit with those changes.
Waiting for accepting the pull request :)

@SkyleKayma
Copy link

Where are the changes about what I have notified before ? (findViewById, apply, retrofit 2, 0f instead of 0.toFloat(), tools:replace instead of tools:ignore ...)

@AndroidDeveloperLB
Copy link
Author

@SkyleKayma I don't know what you are talking about. Please explain.

replaced findViewById with synthetic ones of Kotlin.
@AndroidDeveloperLB
Copy link
Author

@SkyleKayma But speaking of findViewById, I decided to replace them all :)

@@ -22,6 +21,6 @@ android {
dependencies {
compile fileTree(dir: 'libs', include: ['*.jar'])
compile project(':library')
compile 'com.android.support:appcompat-v7:25.1.0'
compile 'com.android.support:appcompat-v7:27.1.1'
compile 'com.squareup.retrofit:retrofit:1.9.0'

Choose a reason for hiding this comment

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

Pls go with new version since everything has been updated, why not that?
implementation 'com.squareup.retrofit2:retrofit:2.4.0'
And change all "compile" by "implementation".

Copy link
Author

@AndroidDeveloperLB AndroidDeveloperLB May 8, 2018

Choose a reason for hiding this comment

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

Makes sense. Added.
The retrofit one is problematic. API has changed, and I don't want to see what should be changed just for this. Need to do other work... Will stay with 1.9.0 for now.

Choose a reason for hiding this comment

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

Ok ok ;D

}

override fun onLoad(periodIndex: Int): List<WeekViewEvent>? {
return onMonthChangeListener!!.onMonthChange(periodIndex / 12, periodIndex % 12 + 1)

Choose a reason for hiding this comment

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

How can you be sure that "onMonthChangeListener" will not be null here ?
I think that nullable "?" is better. It returns nullable List btw.

invalidate()
}
private var mTimeTextPaint: Paint? = null
private var mTimeTextWidth: Float = 0.toFloat()

Choose a reason for hiding this comment

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

don't use this :
0.toFloat()
Use this instead :
0f
If you can do the change everywhere

Copy link
Author

Choose a reason for hiding this comment

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

Right. This was part of the auto-conversion. Fixed that too.

canvas.drawRect(start, startY, startPixel + mWidthPerDay, height.toFloat(), futurePaint!!)
}
} else {
canvas.drawRect(start, mHeaderHeight + (mHeaderRowPadding * 2).toFloat() + mTimeTextHeight / 2 + mHeaderMarginBottom, startPixel + mWidthPerDay, height.to

Choose a reason for hiding this comment

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

Try to avoid usage of reserved words. If you could rename this by something like "value", it could be great.

constructor(id: String, name: String, startYear: Int, startMonth: Int, startDay: Int, startHour: Int, startMinute: Int, endYear: Int, endMonth: Int, endDay: Int, endHour: Int, endMinute: Int) {
this.identifier = id

this.startTime = Calendar.getInstance()

Choose a reason for hiding this comment

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

In Kotlin you have the "apply" function that allow you a better way to do:
this.startTime = Calendar.getInstance().apply{ set(Calendar.YEAR, startYear) set(Calendar.MONTH, startMonth - 1) set(Calendar.DAY_OF_MONTH, startDay) set(Calendar.HOUR_OF_DAY, startHour) set(Calendar.MINUTE, startMinute) }

Choose a reason for hiding this comment

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

Let a line jump between lines, can't do it on Github.

Copy link
Author

Choose a reason for hiding this comment

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

Seems ok. Done

startActivity(intent)
}

findViewById<View>(R.id.buttonAsynchronous).setOnClickListener {

Choose a reason for hiding this comment

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

Same here, don't use findViewById

val intent = Intent(this@MainActivity, AsynchronousActivity::class.java)
startActivity(intent)
}
findViewById<View>(R.id.buttonWholeViewSnap).setOnClickListener {

Choose a reason for hiding this comment

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

Same here, don't use findViewById

weekView.setMaxTime(24)
weekView.isScrollNumberOfVisibleDays = true
setDayViewType(TYPE_WEEK_VIEW)
val cal = Calendar.getInstance()

Choose a reason for hiding this comment

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

Use "apply"

override fun onOptionsItemSelected(item: MenuItem): Boolean {
if (item.itemId == R.id.action_today) {
val cal = Calendar.getInstance()
cal.set(Calendar.DAY_OF_WEEK, cal.firstDayOfWeek)

Choose a reason for hiding this comment

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

Use apply

Copy link
Author

Choose a reason for hiding this comment

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

Fine. Though I think the original code is easier to read.

<LinearLayout
xmlns:android="http://schemas.android.com/apk/res/android" xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent" android:layout_height="match_parent" android:background="#fff"
android:orientation="vertical" tools:context=".MainActivity">

Choose a reason for hiding this comment

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

If you could avoid to put all values in the same line, it could be great

Choose a reason for hiding this comment

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

Same for after

Copy link
Author

Choose a reason for hiding this comment

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

It's just the style we have in other projects. Can I set it to be the standard one just in this project? I don't want to set a global rule...

Choose a reason for hiding this comment

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

Let's push like this, I could do this later ;)

Copy link
Author

Choose a reason for hiding this comment

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

I actually prefer the way it is now, because it's easier to see view hierarchy even in XML. Many times, when a view has a lot of attributes, it causes me to scroll a lot and try to remember what's happening around. Hard to read and understand what's going on, while also seeing the attributes.

But I do understand the request: it's the standard. In the past I also wanted to use WhiteSmith code style for Java, but I left it in the end because I couldn't convince anyone to use it (I still think it's better than all the rest).

@SkyleKayma
Copy link

SkyleKayma commented May 8, 2018

Oh ! My bad, I totaly forgot to submit the review x) Now it's done !(Some comments are outdated now, because I wrote this Yesterday x) )

Removed old fonts, using Lato font, which is more modern and used on Android's latest versions.
fixed issues of fonts, which didn't apply to all texts.
getting fonts by using the support library.
@AndroidDeveloperLB
Copy link
Author

ok done with all the comments you've written, and also fixed some of my own.

}

override fun onLoad(periodIndex: Int): List<WeekViewEvent>? {
return onMonthChangeListener!!.onMonthChange(periodIndex / 12, periodIndex % 12 + 1)
Copy link

@SkyleKayma SkyleKayma May 8, 2018

Choose a reason for hiding this comment

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

How can you be sure that "onMonthChangeListener" will not be null here ?
I think that nullable "?" is better. It returns nullable List btw.

Copy link
Author

Choose a reason for hiding this comment

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

I think you are both correct and incorrect. For some reason the library has code to check it that when you use it, you must set a listener:

    if (weekViewLoader == null && !isInEditMode)
        throw IllegalStateException("You must provide a MonthChangeListener")

and then when you set it:

var monthChangeListener: MonthLoader.MonthChangeListener?
    get() = if (weekViewLoader is MonthLoader) (weekViewLoader as MonthLoader).onMonthChangeListener else null
    set(monthChangeListener) {
        this.weekViewLoader = MonthLoader(monthChangeListener)
    }

So you can't escape it from being non null.

But why this requirement exists at all?
How odd...
I think it's better to remove the requirement, and check for null...

For now I will let it stay how it is.

@SkyleKayma
Copy link

You did great dude. Could you take a look just above ?

@AndroidDeveloperLB
Copy link
Author

Done. Please, if you know how to fix the snapping issue, let me know. I've worked a lot on this library all around, but couldn't find why this occurs.
I also need to do some very special customizations that I don't think should be in the library. I've contributed what I can and think that should be here.

…of current time&date. You can test it by setting the time of the day to be 23:59, and wait a minute...

cleaned code a bit.
added getters to some of the inner fields, so that you can put views on top of the WeekView on the correct places.
replaced the time auto-update mechanism that used Handler to update every minute. Now uses proper BroadcastReceiver instead.
fixed issue of not getting correct lastVisibleDate.
@AndroidDeveloperLB
Copy link
Author

I've fixed a lot of issues and improved performance by about x2 of the library (mostly by using cached results). I'm sure there are more that can be optimized.
I think I will have a break now to work on other things.
Hope I will be able to add RTL support too, but this could take a long time.

… there are multiple of them from same calendar, it looks bad.

minor performance improvements.
… there are multiple of them from same calendar, it looks bad.

minor performance improvements.
…id-Week-View into develop

# Conflicts:
#	library/src/main/java/com/alamkanak/weekview/WeekView.kt
Updated to use Android-X instead of support library.
@nivritgupta
Copy link

@AndroidDeveloperLB is your kotlin library has drag and drop event features ? , I noticed someone trying to implement drag and drop features but its not actually working with existing events , do you have any idea how we can achieve this feature ?

@AndroidDeveloperLB
Copy link
Author

@nivritgupta I didn't implement this feature, and when I converted (and before), I've noticed it made the UI very slow, so I think the implementation of it is very flawed and I don't recommend using this feature in its current form.
You are free to try, of course. It's available on the sample of my repository, just on the part that showed the original sample that I've forked from...

@nivritgupta
Copy link

nivritgupta commented Mar 24, 2019

@AndroidDeveloperLB did you know any alternative calendar that have the same functionality like android week view with drag and drop events features ? I am trying to implement drag and drop features in "android week view" but I think its very hard to achieve this functionality with current coding structure .

@AndroidDeveloperLB
Copy link
Author

@nivritgupta Sorry, I don't.
It was hard enough to even find a library to show the calendar nicely.
If you have a lot of time, I suggest considering making your own solution, because I think it might be hard to force something so complex to work the way you want. Plus, no matter how much I worked on this, I've always found somewhere that could be fixed or improved...

@reilem
Copy link
Member

reilem commented Mar 24, 2019

Sorry for sitting on this PR for so long, but as you can probably understand a large PR like this is quite hard to review and we're afraid that the library might no longer behave as we expect it to. However, a kotlin conversion is definitely necessary for the future of this library and so I hope I find the time to test this. I've looked over everything quickly for now and I only have a few questions/concerns:

  • I've seen the usage of double bang (!!) operators here and there. While I know these can work in certain cases I would like to avoid these as much as possible since it increases the risk of a null causing a crash. Even if you claim to have "checked it somewhere else", or that the exception gets "caught", these really damage the modifiability of the library. If someone accidentally removed these checks it can cause unnecessary side effects.

  • Does this PR also fix API 28 compatibility like the PR Fix API 28 Compatibility #119 does?

@AndroidDeveloperLB
Copy link
Author

AndroidDeveloperLB commented Mar 24, 2019

@reilem Ok I have comments to the comments:

  • The conversion to Kotlin itself has replaced all Java files, so it makes sense that a lot has changed. Everything has changed.
  • I didn't only convert to Kotlin. I've also fixed a lot of bugs and improved performance.
  • The !! is used like on Java, so there is no need to worry about NPE more than what you worry about it on Java. In fact, because I converted to Kotlin, I had to go over many places to make sure the conversion is correct, because nullability is one of the main issues when converting to Kotlin.
  • About the fix of API 28, already fixed. In fact, I'm the one who asked about what's the best way to fix it, here . Was much long before the post you've shown ... :)

I think you should embrace this change. Sooner or later it's nicer to have it all in Kotlin. I think what i did made the library more mature.

I've also written where it can be better, what else should be fixed...

@reilem
Copy link
Member

reilem commented Mar 24, 2019

Alright thanks, currently we are working quite heavily on another project but if I have time I will try to locally merge your PR and test all important functionality.

As for the double bang operator, my concern is exactly what you stated. Kotlin adds nice functionality and lots of improvements, and we shouldn't rely on old java logic for null checking. Since all you need to do is add a != null check and it will be automatically casted to a non-optional. I will probably make this change either way so don't worry if you don't have time to do it.

@AndroidDeveloperLB
Copy link
Author

@reilem It's not that you should avoid "!!". It's just that it means that you are aware of what you are doing, that it shouldn't be null when you reach there.
Of course things can get wrong, but the same things could be wrong even if you add null check everywhere. It really depends on the case.
This is especially true for custom views, where almost everything is on a single thread anyway (the UI thread).
What do you mean by "I will probably make this change either way so don't worry if you don't have time to do it." ?

@reilem
Copy link
Member

reilem commented Mar 24, 2019

All I really want to say is that adding a != null check is simple, easy and takes literally 2 seconds and avoids a many great deal of issues in the future. It allows for graceful degradation instead of simply crashing the app and should be used whenever possible. If needed you can throw an exception when it is null so that developers are still able to detect the issue.

I have looked at this situation and understand that right now it seems obvious to you, but to future contributors this sort of coding behaviour can be very confusing. I have been in a few situations where a new programmer joins a team and causes these types of bugs due to previous developer negligence.

And what I mean is that since I am very anal about these type of safety measures haha. And that there's a good chance I will be adding the != null check because it is simply my coding philosophy.

Edit: I’ve removed some of our comments below so that this thread stays on-topic.

@AndroidDeveloperLB
Copy link
Author

@reilem It really depends on the case. Adding null checks everywhere doesn't make sense in all cases.
And sorry, but I don't know the term " I am very anal" .

@Quivr Quivr deleted a comment from AndroidDeveloperLB Mar 25, 2019
@Quivr Quivr deleted a comment from AndroidDeveloperLB Mar 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants