Skip to content
This repository has been archived by the owner on Nov 8, 2021. It is now read-only.

Issue185 v2 #10

Closed
wants to merge 17 commits into from
Closed

Issue185 v2 #10

wants to merge 17 commits into from

Conversation

saidllen
Copy link

No description provided.

@@ -148,8 +148,15 @@ public synchronized Record findLastRecord(String gatewayName) {
private String senderNumber;
private String senderName;
private String gateway;
private String id;
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be better called smsId or messageId, so that it's not confused with Record ID.

@kparal
Copy link
Owner

kparal commented May 6, 2014

Next time please modify just src/esmska/resources/l10n.properties, you don't need to rebuild other properties files (and even if you do, you don't need to include them in your commit).

For some reason I haven't received an email notification about your new patch, I'll have a look at it shortly.


/** Shortcut for this(number, text, gateway, name, senderNumber, senderName, date, null);*/
public Record(String number, String text, String gateway,
Copy link
Owner

Choose a reason for hiding this comment

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

Nothing in the source code uses this constructor, so there's no reason to keep this "legacy" constructor around.

@kparal
Copy link
Owner

kparal commented May 12, 2014

Your patch can't be merged with master, there are conflicts in LegacyUpdater.java. Please update master and rebase your branch on top of it. Like this:

git checkout master
git pull
git checkout issue185_v2
git rebase master
# fix conflicts
git commit
git push

Please exclude l10n_* files.

@kparal
Copy link
Owner

kparal commented May 14, 2014

So, information number 1: If you add some commits but don't add any new comment, I'm not notified by github. That means you should add a new comment if you want to notify me :-) I didn't know either.

Information number 2: It seems you haven't rebased your branch on top of current master, there are still merge conflicts. I can deal with it of course, but it's slightly inconvenient and it makes it harder to try out the code. Please rebase the branch (see above) or at least merge it (might be a bit easier):

git checkout master
git pull
git checkout issue185_v2
git merge master
# fix conflicts
git commit
git push

If there are no updates when running git pull, it means you need to sync your forked repository from my repository. Here's how:
https://help.github.com/articles/syncing-a-fork

@@ -710,15 +711,35 @@ private boolean saveAll() {
}
}

/** Saves history of sent sms */
/**
* Saves history of sent sms
Copy link
Owner

Choose a reason for hiding this comment

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

It's preferable if you don't reformat lines that don't need to be reformatted.

@kparal
Copy link
Owner

kparal commented May 14, 2014

All in all, it looks good. If you correct the small issues, rebase it on current master, and ideally also manage to exclude l10n_*.properties files, it's ready to be merged. I can do the last two things mentioned, if you have big troubles working with git. I understand it needs some experience and it's not particularly easy for beginners.

@saidllen
Copy link
Author

Hopefully all problems are fixed, but I am trying and trying to rebase the branches and exlude the files, but it is not working :( When I modify the file and run rebase --continue, it says the same and show the same version as before, so I modify it again and again and still not working... Sorry for that

@saidllen
Copy link
Author

I hope I did not make it worse, there are so many commits :-D

kparal added a commit that referenced this pull request May 14, 2014
When a message is split into several fragments (to be sent separately),
they are no longer visible as separate pieces in the history (which
makes it hard to forward the message), but they are instead visible as
the original single message.

Fixes: https://code.google.com/p/esmska/issues/detail?id=185
Contributed by: Lenka Saidlova <lenkasaidlova@seznam.cz>
Pull request: #10
@kparal
Copy link
Owner

kparal commented May 14, 2014

Yes, you did it correctly. As for the excluding files, don't worry, that will come with time :-)

Thank you for your patch! I committed it as 211ce82.

@kparal kparal closed this May 14, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants