Update AgendaSkinTimeScale24HourAbstract.java #40
Conversation
The displayedDateTimeInvalidationListener invalidation listener was firing twice when the localDateTimeRangeCallback runs. I replaced it with a change listener. The change listener only fires once.
|
Are all the tests still green? I'll check myself. |
|
Test still are green. Added a special one for displayed date. Released as 8.0-r5-SNAPSHOT. I did not merge this PR because it reformats half the source. Thank you. |
|
I didn't check the tests. I am using Agenda in custom code and I was seeing the problem with the invalidation listener firing off twice when I only want one. I don't understand how changing this one listener reformats half the source? Can you please explain? |
|
You PR reformatted a lot of code, I have no idea why. On 27-9-2015 14:48, daviddbal wrote:
|
|
It was my first time doing a PR. Maybe I did something wrong. I should take a look at the tests. Is there a test that sets the localDateTimeRangeCallback? In my version, which is a old, it was null until I set it. |
|
David, if you are using Eclipse or some other IDE, remove the save actions that reformat the code. Tom, you should put out a style guide for your library that developers can import. Or at least put the main rules on the ReadMe (tabs/spaces, brackets on new lines) |
|
Yeah, well, formally there is no single styleguide, but you are right. David, the change is already in there. |
|
OK. Thanks for taking care of the change. By the way, I added repeatable appointments to agenda. Is that a feature you want to offer or is it beyond the scope of your project? |
|
Yes, it was something I was looking into. There is a standarisation for repeating appointments. But the control itself should not need any specific support for it. If an appointment is changed and it should reflect on others, the logic providing the control with appointments should take care of that. So I am curious what you added for repeatable appointments, yes. On 27-9-2015 20:27, daviddbal wrote:
|
|
I did a couple of things to make repeatable appointments work. First, I split the Appointment interface into two parts. One is called Repeatable, which contains all the parts of an appointment that can be shared by a repeat rule. Appointment now extends Repeatable. Then I made a Repeat class that contains all the repeat rules and methods to generate appointments. When editing appointments the user can choose to apply changes to one, all or future appointments. Repeatable appointments are created and destroyed automatically depending on the start and end date of the current skin, which is why I needed the localDateTimeRangeCallback. Functionally, I largely follow Google Calendar for editing repeatable appointments. Repeatable appointments was surprisingly complex. It took a couple of months and about two thousand lines of code. I am pleased with it, but I expect my implementation could be fine-tuned. Could the Jfxtras community help polish it up? One thing I need is test code. I have not written any test code. I test it manually as I go. I read an Agile development book once, but I guess it didn't sink in. I would be happy to share it with you, but I have made my own version that is somewhat integrated into my own custom code. I need to get a ordinary version of agenda and insert my parts and make sure it still works. Currently, I am having trouble with Gradle. For example, I just made a clone of the JFXtras github project and building on the command line produced the error "When running gradle with java 8, you must set the path to the old jdk, either with property retrolambda.oldJdk or environment variable JAVA5_HOME/JAVA6_HOME/JAVA7_HOME" How should I share my code? Should I commit a branch to the Jfxtras Github repository? If you can give me some tips on getting my work integrated I would appreciate it. |
|
I got the retrolambda problem solved, but I get a failure on one test case: AgendaMouseManipulateTest.java:194. Is that a problem? I am amazed to see that you programmed simulated mouse actions in the test cases. I need to figure out how to do that. |
|
Questions: I am having trouble getting JFxtras working properly. I had some of these problems a few months ago which is one reason why I started my own project. Below are some issues. Can you help me?
|
|
Good and enough tests is the only way to keep UI components stable. For every use case or bug report that I get I write a test, so the coverage will grow over time. For example your change listener improvement resulted in a test testing if changing the displayed date indeed repopulates the control. Concering the failing test; yes, that is a problem. It does not fail here, so something is different and possibly broken. Maybe not, but that is what the test is for. Test should always be green, either by fixing the code or the test. I'm still not clear what you changed exactly, so I would be interested in seeing the code. As I see it, the repeatable part is the responsibility of the domain logic providing the appointments to Agenda. The only requirement I can see Agenda may have, is visualizing a repeating appointment by assigning a class to it. Trails are exactly what they say; stand alone classes that simulate a certain situation. Intended for manual testing and should be able to run. This probably is a Eclipse setup thing you have and hard to diagnose from here. The 2nd issue also suggest that is the problem. TestUtil is part of the test-support artifact of JFXtras. |
|
Agenda doesn't know the difference between a repeatable appointment and a individual appointment. All the overhead is handled by other classes that I wrote. Certainly, it is best for you to see it. In a few days I hope to share with you a working demo. I think you have good ideas about test cases, particularly for UI code. I will start making some as needs arise. I like your approach of building them step-by-step. I still want to get my development setup proper. As I said before, I can't run your trials because Eclipse doesn't see the main. Also, the references are going to my libraries not the classes in the project hierarchy. I started with a clone of the entire Github project (https://github.com/JFXtras/jfxtras.git). Then, I did "gradle eclipse" in a terminal. Then I imported the project into Eclipse. Next, I copied the files from jfxtras-src-main-java-jfxtras into a new project in order to match the package names. That way Eclipse recognized the class locations and didn't go to the libraries. However, that means I can't put my code back to Github without coping it back to the original cloned folder. Its a clumsy work-around. Do you know what I am suppose to do? |
|
That makes sense, repeatable appointments could be an add-on. I would not do it like that. I use a gradle statement that downloads all dependencies into a subfolder in the project and then setup Eclipse manually. In that way you can decide what is in the classpath and what not. What you probably should have done is fork jfxtras and then work on top of your own copy. |
|
Repeatable appointments is mostly an add-on. Some changes were necessary in the main agenda code, but not much. For example I replaced your AppointmentMenu popup with one that allows configuration of repeat rules. I used FXML for that popup. I see you don't have any FXML, so I hope there isn't a problem including it. I will try to figure out how to make a fork on Github. The best I could do was make a clone. Maybe that started my problem. Most of the time I use Subversion, so I don't know much about Git or Github. |
|
There is no need to do that, Agenda supports a custom popup menu, so it can be provided by the addon. On 28-9-2015 19:21, daviddbal wrote:
|
|
I saw that feature, but all appointments need the ability to be assigned repeatable rules so why leave the non-repeatable popup? When you see the demo you can decide. |
|
Here is a link (https://bitbucket.org/daviddbal/repeatable-agenda) to the repeatable agenda demo. I look forward to your thoughts. |
The displayedDateTimeInvalidationListener invalidation listener was firing twice when the localDateTimeRangeCallback runs. I replaced it with a change listener. The change listener only fires once. I suppose its name should change from displayedDateTimeInvalidationListener to displayedDateTimeChangeListener as well.