Introduce synchronize merge method #1992

ckieschnick · 2018-05-24T14:32:13Z

We're currently developing a sharing extension to KeePassXC which we like to contribute. During the development we discovered some problems which need to be addressed beforehand. These needed fixes and features are part of this pull request to allow you to review the new and changed behavior.

The changes are mainly concerned with ensuring to have consistent and expected modification timestamps and states since synchronized merge itself does not modify timestamps.

Description

We extracted the whole merge logic into a dedicated class Merger.

It encapsulates the existing merge strategies and the new synchronized strategy to allow merges using the entry history.
It takes care, that the precision of serialized and runtime data is consistent (at least for synchronized merge) to ensure deterministic merge results.
It prevents unintended changes of modification times by explicitly deactivating the modification tracking while merging.
It synchronizes the deletions between databases.
It implements a (very simple) change tracking during merge to track modifications to the database.

Additionally, we needed some tooling, especially comparison methods:

Compare encapsulates generic comparison methods and flags
Objects like Entry, Group, TimeInfo, ... provide equals. == and != with additional options to allow for more sophisticated comparisons i.e. excluding location changes or excluding user statistics

Furthermore we introduced Clock which can be stubbed during tests with TestClock to remove the dependency to the system time during tests.

Motivation and context

There are currently a number of tickets related to sharing and synchronizing of databases which may profit using the changed implementation: supports #1152, #841, #637, and #90. Resolves #939 and resolves #818.

How has this been tested?

Tested using and extending the test suite and manually with code which will be part of a later contribution.
The code contains some TODO which point out some shortcomings, extension points or potentially surprising behavior.

Types of changes

✅ Bug fix (non-breaking change which fixes an issue)
✅ New feature (non-breaking change which adds functionality)
✅ Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]
✅ My change requires a change to the documentation and I have updated it accordingly.
✅ I have added tests to cover my changes.

EDIT @phoerious: Add "resolves" keywords in front of issue numbers

TheZ3ro · 2018-05-25T10:27:57Z

src/core/AutoTypeAssociations.cpp

+        return false;
+    }
+    for (int i = 0; i < m_associations.count(); ++i) {
+        if (m_associations[i] != other.m_associations[i]) {


This checks that two lists share the "same" elements in an order-preserving comparison, but I don't think that this is what we want here.

An AutotypeAssociations is a list of Association like [ Association1, Association2, Association3 ].
I think the comparison should be order-less.

Should the above example be considered == to [ Association2, Association3, Association1 ] ? What do you guys think?

In my opinion the above example should only be considered equal when there is no code which depends on the order of the associations. Since it is about auto typing, I assume that is or there may be some feature which does execute the first association by pressing a shortcut - in this case we have to compare order preserving.

droidmonkey · 2018-06-01T18:32:00Z

I may be dense, but how do you use this feature? I compiled the application and everything seems the same to me.

Also the merge function now duplicates folders, including the recycle bin.

ckieschnick · 2018-06-05T06:56:29Z

The application should behave the same as before since this pull request mainly considers improvements to existing code. This should allow you to review the changes more easily. The few new features are currently not connected to any UI, so they are only usable writing new code like tests.

I'll try to look into the issue regarding the recycle bin - can you describe the merge scenario when this happens? Currently I cannot see anything out of ordinary. The only scenario in which "folders" seem to be duplicated is a merge of two folders which do not share the same history (have different UUID but the same appearance - which seems to be the intended behavior).

phoerious · 2018-06-08T08:12:45Z

The only scenario in which "folders" seem to be duplicated is a merge of two folders which do not share the same history (have different UUID but the same appearance - which seems to be the intended behavior).

I think this may happen quite regularly with recycle bins. But for this particular case it may be solved by checking which UUID corresponds to the recycle bin in either database and then treat them as equivalent.

phoerious

Thank you for you massive contribution. I marked a few things to change which I noticed after a first read-through.

Overall, you contribution is definitely one of the higher-quality ones we have received so far. I think the most important issue is that your patch set contains quite a few unnecessary diffs which only change the formatting of some files. Please remove those. The changes may be correct regarding our style guide, but are not the responsibility of this PR. They would only increase the chance of merge conflicts and inflate the patch set unnecessarily, so please remove them.

It would also be great if you could add a few doc blocks above newly introduced classes and methods briefly stating what they are for and what their parameters are (especially for the more generic interfaces, which might need some explaining).

phoerious · 2018-06-08T09:21:28Z

src/browser/BrowserService.cpp

@@ -567,7 +569,8 @@ QList<Entry*> BrowserService::sortEntries(QList<Entry*>& pwEntries, const QStrin
            // Sort same priority entries by Title or UserName
            auto entries = priorities.values(i);
            std::sort(entries.begin(), entries.end(), [&priorities, &field](Entry* left, Entry* right) {
-                return QString::localeAwareCompare(left->attributes()->value(field), right->attributes()->value(field)) < 0;
+                return QString::localeAwareCompare(left->attributes()->value(field), right->attributes()->value(field))
+                       < 0;


I would prefer you don't make so many small formatting changes if they don't improve something substantially. They primarily make merge conflicts more likely.

Moreover, I know there are style guides which dictate a fixed maximum line width, but IMHO breaking only a few characters into a next line is overall less readable than having slightly longer lines at times (especially considering that most people have fairly wide screens these days anyway).
If you break more than just a single term component, it's okay, but this here appears a little excessive to me. If anything, break after the first comma or so.

phoerious · 2018-06-08T09:25:40Z

src/core/AutoTypeAssociations.h

@@ -46,6 +46,12 @@ class AutoTypeAssociations : public QObject
    int associationsSize() const;
    void clear();

+    bool operator==(const AutoTypeAssociations& other) const;
+    bool operator!=(const AutoTypeAssociations& other) const


Maybe add inline?

phoerious · 2018-06-08T09:29:46Z

src/core/Clock.cpp

+ */
+#include "Clock.h"
+
+static const Clock* m_clock = nullptr;


Please make this a (static) member variable of type QScopedPointer. Don't use raw pointers for long-lived managed freestore objects. You could also use QSharedPointer and then let instance() return that instead of a reference (perhaps even the better because more explicit variant).

phoerious · 2018-06-08T09:33:44Z

src/core/Compare.cpp

+
+#include <QColor>
+
+bool operator<(const QColor& lhs, const QColor& rhs)


This operator needs a doc block explaining what it does and what the order criterion is. It might also be sensible to move it out of core/ and into gui/.

This code is needed to compare QColor properties of entries/groups, so it is not part of gui.

QColor is part of QtGui, so it will pull in the GUI libs. That was my intention. But you're right, Entries already depend on it, so I'm okay with leaving it here for now.

phoerious · 2018-06-08T09:34:31Z

src/core/Compare.h

+
+#include "core/Clock.h"
+
+enum CompareOption


This might need namespacing or a more specific name that points to its purpose of comparing entries and groups (since the enum members are quite specific to that).

For now I don't have any better name for CompareOption. For now it is only a way to collect all possible options for comparisons to prevent scattering and duplicating the logic all over the classes. This way, the client is responsible to select the usable and appropriate members but has consistent behavior and api for comparisons. For now I'm out of ideas for better naming or a better implementation strategy, but I'm open for proposals and improvements in this regard.

The members of this enum are quite specific to database items (i.e. groups and entries). Perhaps make that part of the name.

phoerious · 2018-06-08T10:59:04Z

src/core/Merger.cpp

+    const auto sourceHistoryItems = sourceEntry->historyItems();
+
+    QMap<QDateTime, Entry*> merged;
+    for (Entry* historyItem : targetHistoryItems) {


Shouldn't the merge result of two histories just be the time-ordered set of both? I don't think individual history items ever change (unless a client has done something shady).

That's the implemented strategy. It does look more complex since I do some assertions additionally to the needed boilerplate code to prevent unintended database changes.

Why not simply use QSets then?

QSet does not allow to specify arbitrary equality operators - it just uses qHash. Defining qHash using timestamps (and maybe uuid) only may lead to unintended behavior when defined globally (every QSet would treat entries equal regardless of object identity) or locally (different semantic of QSet<Entry>). In my opinion QMap states the intend more clearly and provides the needed characteristics.

Okay, fair point. One last comment: you should perhaps merge lines 376 and 377 into an else if construct to make it clearer that both conditions are mutually exclusive.

phoerious · 2018-06-08T11:02:29Z

src/core/Merger.h

+
+class Merger
+{
+    Q_DECLARE_TR_FUNCTIONS(Merger)


Any reason not to make this a QObject?

QObject is not needed to work properly for the merger - at least not in the current design as short living utility. QObject would add just unnecessary compile time overhead.

I don't see the overhead to be problematic compared to the benefits of being able to use the meta object system, signals, slots, and generally more uniformity.

I simply don't see any benefits. Without inheriting, the API reveals the only intend the class for and how it should be used. The clutter using the meta system somewhat hides this and suggests uses which the class was not designed for. Currently, adjusting to QObject is trivial, but I'd like to have the person changing to inherit to QObject reason about the consequences (memory handling, object lifetime, ...). That's why I would prefer Mergerto be a simple class instead QObject based.

I am mostly arguing from a consistency standpoint here. This class is not performance-critical, so this bit of overhead rarely matters. We have a few classes which are not derived from QObject and for some it's vital, for most it's quite confusing. I've run into the situation often enough that I wanted to use a QMetaObject function on an instance of a class only to realise that it does not inherit from QObject. Adding QObject later is not as easy as you might think. Sometimes incompatible inheritance of usage patterns have evolved which makes it impossible to let a class inherit from QObject later without refactoring large parts of the code. For a Merger class, I could very well imagine use cases where you would want to use signals and slots later on. You are already using the translation functions.

To be honest, my experience is, that I don't want to use QObject as long I don't have to. Since the change is trivial (for now), so I'll go along and inherit from QObject.

I won't fight to the death about this one, but I think consistency is a good thing here.
For objects which will exist many times in memory, I would agree with you, though.

phoerious · 2018-06-08T11:04:38Z

src/proxy/NativeMessagingHost.cpp

 {
    m_localSocket = new QLocalSocket();
    m_localSocket->connectToServer(getLocalServerPath());
    m_localSocket->setReadBufferSize(NATIVE_MSG_MAX_LENGTH);
-


Please remove all these files from the diff which have no changes except minor formatting.

phoerious · 2018-06-08T11:05:59Z

tests/TestMerge.cpp

 #include "core/Metadata.h"
 #include "crypto/Crypto.h"

 QTEST_GUILESS_MAIN(TestMerge)

+namespace
+{
+    TimeInfo modificationTime(TimeInfo timeInfo, int iYears, int iMonths, int iDays)


years, months, days

phoerious · 2018-06-08T11:20:16Z

tests/stub/TestClock.h

+
+namespace Test
+{
+    class Clock : public ::Clock


Why not just call this class ClockTest or TestClock? Needing to refer to the super class as ::Clock is confusing.

Since only Test::Clock references ::Clock it seems easier to write Test::Clock (shows that the object should only be used in tests) instead of TestClock/ClockTest (may collide with a potential test for Clock) or Test::TestClock (redundant naming) in the tests.

I don't like this naming. It's against all our conventions. Wrap it in an anonymous namespace if you are worried about name clashes, but ::Clock is not the way to go.

ckieschnick · 2018-06-11T10:30:56Z

Thanks for your feedback. I'd like to add some more general thoughts:

It would be nice to limit the scope - since this PR is just a part of the final contribution, we want to concentrate on the final feature. Providing transactions for merges or merging recycle bins are features we'd like to see too. Since KeePassXC was able to work without them up to now, I think that it is legitimate to push them out into their own tickets.
Concerning the comments about formatting - I did make sure to run make format before contributing my changes. Every file which contains only format changes was changed by the command. In my opinion it would be tedious to run the formatting and than reverting all unnecessary changes by hand. I suggest, that you run make format over the whole code base in develop and make every PR pull that commit. In most cases, there would no or only trivial merge conflicts (and the more hairy ones are hopefully easy to resolve for the contributor). This way we save time for everybody. You'll just have to look for real style issues (naming and structuring of code) and your contributors do not have to worry about those comments. In some cases, auto format does make lines less readable (awkward line breaks after member return types, conditions with line breaks like you commented above). The solution would be to tweak the formatter or to adjust for the reader. Manually tweaking the auto format before commit seems to be the wrong way.

phoerious · 2018-06-11T11:49:09Z

I did not suggest adding a new transaction feature. I merely asked for starting one with beginUpdate() and ending it afterwards with endUpdate() before performing a merge (see code comment above).

We do not require you to run make format at all at this point. We had a PR for reformatting everything with #1191 at some point, but I don't think it caught everything. Stick to our conventions for your own code and let us deal with reformatting the rest at a later point ourselves. When we decide to enforce formatting, we will add it as part of our CI tests.

ckieschnick · 2018-06-11T13:12:27Z

On the one side, it seems like I misunderstood "transaction" - on the other side, maybe it would be a good concept for the future (you already have some parts which do transaction the hard way like the edit widgets).

Regarding formatting. I plead to format the code base consistently as soon as possible. I believe a lot of contributers are working on different projects, each with its own style guide. It is much easier to switch between projects and their code styles when the existing code serves as example than reading the style guide before or after writing code. This way one sees immediately when the new code deviates from the code style since it looks odd beside the existing code.
Since I'm going PR more code with even more changes, I'm really horrified to have half of the comments to be style related instead of discussing about implementation strategies, drawbacks and side effects - and - I believe so are you ;)

phoerious · 2018-06-11T13:18:07Z

On the one side, it seems like I misunderstood "transaction" - on the other side, maybe it would be a good concept for the future (you already have some parts which do transaction the hard way like the edit widgets).

Good point. We should think about that.

Regarding formatting. I plead to format the code base consistently as soon as possible.

Well, we have and you see that your make format did not totally screw up everything (only a few files). I can make another PR after this which reformats the remaining parts.

It is much easier to switch between projects and their code styles when the existing code serves as example than reading the style guide before or after writing code.

True. We had to tell people to move } and else on the same line etc. a lot. Most of that has resolved itself by now. As I said, at some point in the future, we will add make format to our CI process, so you will get yelled at by TeamCity and not by me anymore. ;-)

ckieschnick · 2018-06-13T09:47:27Z

The last commit should fixes the error for running TestGui (the testdata cover a case, I thought would never occur). Furthermore I splitted/renamed the merge strategies - the old KeepNewer is now basically the Synchronize method which unifies the history with the newest value on top. SynchronizeUsingRemote/SynchronizeUsingLocal will force the corresponding value on top by duplicating it if needed. I took the liberty to introduce OverwriteUsingNewer, OverwriteUsingLocal and OverwriteUsingRemote which basically just take the appropriate entry and discarding the other one completely - similar to the old KeepNewer and KeepExisting to keep that functionality for now. KeepBoth was not touched.

I still have some doubts about the merge of deletedObjects, since they were not considered up to now:

the synchronize strategies should be safe, since deletions are ignored when the last modification time is newer
KeepBoth may delete entries/groups when the remote database has a deletion time stamp after the last local modification time stamp - is this the intended behavior?
OverwriteUsingLocal and OverwriteUsingRemote will delete the corresponding value if the time stamp of the DeletedObject has a newer time stamp than the overwriting value - a possible solution would to push the old state to history to have a newer value (without changes!) on top

Another point I want to mention is about the merge Currently, each and every group is able to differ in merge behavior from its parent and the rest of the database (at least as soon as the user is able to set this property). Depending, how it should be used, it may allow someone controlling the remote database to influence the local database in surprising behavior because relocated entries may use a different strategy than in their original location.

phoerious · 2018-06-13T12:57:02Z

DeletedObjects should never be overwritten or discarded. They are nothing but a reference to an entries/groups previous UUID (no data attached) and their only purpose is to allow bidirectional remote synchronisation and merging. We don't support remote merging atm, but KeePass does and if a merge strategy suddenly discards some DeletedObjects, it may result in weird and unintended effects when synchronizing the database with a WebDAV server etc. But DeletedObjects are also useful for local merging.

ckieschnick · 2018-06-13T14:08:06Z

I'll have a look at the KeePass code to make sure that we do have compatible behavior.

ckieschnick · 2018-06-14T14:26:16Z

I adjusted the merge strategies to include a history merge (I discarded the Overwrite* stuff and reverted back to Keep*). Like KeePass, Synchronize is the only strategy considering DeletedObjects - the others do not regard them which leads to an (in my opinion) surprising state of the same Uuid in the key-database and the DeletedObjects. Currently, the tests which fail are annotated to highlight this issue.
Another thing we recognized is, that the KeepBoth strategy does reimport entries with the same Uuid into the database in case they were deleted before. This also leads to the state described above - KeePass does not suffer this issue since it duplicates each and every entry/group (I would suggest, that new entries are inserted using a new Uuid when a DeletedObject with the same Uuid exists).

Finally, the state with a Uuid in both entries/groups and deletedObjects is likely to cause issues using the database as source for the synchronization merge since newer deletions are applied immediately after the import, leaving the target database without import entry while the source database does contain them.

Update: Dominik Reichl intended the (for me surprising) behavior (see https://sourceforge.net/p/keepass/bugs/1752/). I'll change the tests accordingly to expect this behavior.

louib · 2018-06-26T20:50:18Z

@ckieschnick thanks for that PR!

I did not have a chance to look at the changes in depth. Will this PR deprecate #1882? If so, do you think you can add the unit tests of #1882 in this PR?

ckieschnick · 2018-06-26T21:50:37Z

The new functionality of this PR does not check if a merge is required. The Merger just registers changes and marks the database modified if needed.

TheZ3ro · 2018-07-03T20:47:37Z

@ckieschnick Please implement all the operators in #1883 so we can close that PR in favor of this one

ckieschnick · 2018-07-11T07:30:54Z

@TheZ3ro The most of the requested comparison functions are implemented within the equals methods of Group and Entry. #1883 does provide some additional functionality which seems questionable to me since I'm not able to reconstruct the scenarios the operators are supposed to be used in (i.e. recursive comparison, only comparing attachments and attributes ignoring uuid etc). Since the merge functionality cannot be improved by them, I'd like to keep both PR separate.

Concerning the implemented operators in #1883, I'd rather not implement the comparison using the == and != as long as it would not be possible to substitute the compared objects in case of equality.

droidmonkey · 2018-07-11T14:46:11Z

I agree with @ckieschnick analysis here.

TheZ3ro · 2018-07-12T16:04:36Z

So I think we can close #1883 since it's obsolete and conflict with this PR

phoerious · 2018-07-24T22:23:56Z

Sorry for the delay. I think this has matured quite a bit now and is ready to be merged into develop for real-world testing. I'll do the final review next week when I'm back home. In the meantime, could you please rebase the branch to the current develop HEAD? Thanks!

droidmonkey · 2018-09-26T01:28:18Z

@phoerious please complete your final review so this can be merged

TheZ3ro · 2018-09-27T14:08:13Z

Seems that there are some files conflicting now

* Create history-based merging that keeps older data in history instead of discarding or deleting it * Extract merge logic into the Merger class * Allows special merge behavior * Improve handling of deletion and changes on groups * Enable basic change tracking while merging * Prevent unintended timestamp changes while merging * Handle differences in timestamp precision * Introduce comparison operators to allow for more sophisticated comparisons (ignore special properties, ...) * Introduce Clock class to handle datetime across the app Merge Strategies: * Default (use inherited/fallback method) * Duplicate (duplicate conflicting nodes, apply all deletions) * KeepLocal (use local values, but apply all deletions) * KeepRemote (use remote values, but apply all deletions) * KeepNewer (merge history only) * Synchronize (merge history, newest value stays on top, apply all deletions)

The changes requested were made

- New Database Wizard [#1952] - Advanced Search [#1797] - Automatic update checker [#2648] - KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739] - Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439] - Remove KeePassHttp support [#1752] - CLI: output info to stderr for easier scripting [#2558] - CLI: Add --quiet option [#2507] - CLI: Add create command [#2540] - CLI: Add recursive listing of entries [#2345] - CLI: Fix stdin/stdout encoding on Windows [#2425] - SSH Agent: Support OpenSSH for Windows [#1994] - macOS: TouchID Quick Unlock [#1851] - macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583] - Linux: Prevent Klipper from storing secrets in clipboard [#1969] - Linux: Use polling based file watching for NFS [#2171] - Linux: Enable use of browser plugin in Snap build [#2802] - TOTP QR Code Generator [#1167] - High-DPI Scaling for 4k screens [#2404] - Make keyboard shortcuts more consistent [#2431] - Warn user if deleting referenced entries [#1744] - Allow toolbar to be hidden and repositioned [#1819, #2357] - Increase max allowed database timeout to 12 hours [#2173] - Password generator uses existing password length by default [#2318] - Improve alert message box button labels [#2376] - Show message when a database merge makes no changes [#2551] - Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790] - Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]

TheZ3ro added this to the v2.4.0 milestone May 25, 2018

TheZ3ro added the new feature label May 25, 2018

TheZ3ro reviewed May 25, 2018

View reviewed changes

phoerious previously requested changes Jun 8, 2018

View reviewed changes

ckieschnick mentioned this pull request Jul 12, 2018

Sharing groups #2109

Merged

droidmonkey mentioned this pull request Sep 19, 2018

Implement comparison operators #1883

Closed

droidmonkey added the PR: Ready for Merge label Sep 26, 2018

droidmonkey force-pushed the feature/history_merge branch from 6d9e673 to 2a90ddc Compare September 30, 2018 13:03

droidmonkey approved these changes Sep 30, 2018

View reviewed changes

droidmonkey mentioned this pull request Sep 30, 2018

Implement synchronize merge code added by #1992 #2339

Closed

5 tasks

droidmonkey merged commit c1e9f45 into keepassxreboot:develop Sep 30, 2018

droidmonkey mentioned this pull request Mar 26, 2019

Date is now localised since 2.4.0 #2882

Closed

ckieschnick deleted the feature/history_merge branch August 12, 2019 16:04


		#include <QColor>

		bool operator<(const QColor& lhs, const QColor& rhs)

Introduce synchronize merge method #1992

Introduce synchronize merge method #1992

Conversation

ckieschnick commented May 24, 2018 • edited by droidmonkey

Description

Motivation and context

How has this been tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

ckieschnick May 28, 2018 • edited

Choose a reason for hiding this comment

droidmonkey commented Jun 1, 2018 • edited

ckieschnick commented Jun 5, 2018 • edited

phoerious commented Jun 8, 2018 • edited

phoerious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jun 11, 2018 • edited

Choose a reason for hiding this comment

phoerious Jun 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckieschnick Jun 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jun 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jun 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jun 8, 2018 • edited

Choose a reason for hiding this comment

ckieschnick Jun 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckieschnick commented Jun 11, 2018

phoerious commented Jun 11, 2018 • edited

ckieschnick commented Jun 11, 2018

phoerious commented Jun 11, 2018 • edited

ckieschnick commented Jun 13, 2018

phoerious commented Jun 13, 2018 • edited

ckieschnick commented Jun 13, 2018

ckieschnick commented Jun 14, 2018 • edited

louib commented Jun 26, 2018

ckieschnick commented Jun 26, 2018

TheZ3ro commented Jul 3, 2018 • edited

ckieschnick commented Jul 11, 2018

droidmonkey commented Jul 11, 2018

TheZ3ro commented Jul 12, 2018

phoerious commented Jul 24, 2018

droidmonkey commented Sep 26, 2018

TheZ3ro commented Sep 27, 2018

ckieschnick commented May 24, 2018 •

edited by droidmonkey

ckieschnick May 28, 2018 •

edited

droidmonkey commented Jun 1, 2018 •

edited

ckieschnick commented Jun 5, 2018 •

edited

phoerious commented Jun 8, 2018 •

edited

phoerious Jun 11, 2018 •

edited

phoerious Jun 8, 2018 •

edited

ckieschnick Jun 11, 2018 •

edited

phoerious Jun 11, 2018 •

edited

phoerious Jun 11, 2018 •

edited

phoerious Jun 8, 2018 •

edited

ckieschnick Jun 11, 2018 •

edited

phoerious commented Jun 11, 2018 •

edited

phoerious commented Jun 11, 2018 •

edited

phoerious commented Jun 13, 2018 •

edited

ckieschnick commented Jun 14, 2018 •

edited

TheZ3ro commented Jul 3, 2018 •

edited