-
Notifications
You must be signed in to change notification settings - Fork 21
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
Git ui /egm integration WIP #198
base: master
Are you sure you want to change the base?
Conversation
fundies
commented
Nov 21, 2020
•
edited by RobertBColton
Loading
edited by RobertBColton
return _gitHistoryModel; | ||
} | ||
|
||
void EGMManager::ResourceChanged(Resource& res, ResChange change, const QString& oldName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method I need help with and is very incomplete
@@ -42,9 +44,12 @@ bool TreeModel::setData(const QModelIndex &index, const QVariant &value, int rol | |||
R_EXPECT(index.isValid(), false) << "Supplied index was invalid:" << index; | |||
if (role != Qt::EditRole) return false; | |||
|
|||
if (!MainWindow::resourceMap->ValidResourceName(value.toString())) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should semaphore-guard this. Do we have threads, yet? I suspect we inevitably will, in the future.
Also, nit: indentation is off by one space? Maybe GitHub's just rendering it wrong.
@@ -7,6 +7,12 @@ | |||
#include <QVector> | |||
#include <string> | |||
|
|||
struct Resource { | |||
QString name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I can see why you're unhappy with Robert's code so far. You're trying to implement something in parallel that could easily be described within the model if we used a message wrapper. Instead of making TreeModel store bare TreeNode
messages, we should probably create a TreeNodeWrapper
that contains the proto and the MessageModel to wrap it, as well as the QString name cache, and the TreeNodeWrapper*
parent. This index should then be added to the tree model itself. Once that's done, we can stop fucking serializing bare TreeNode pointers when sending drag/drop MIME data everywhere, because that WILL kill us, later.
Reverted | ||
}; | ||
|
||
class ResourceChangesModel : public QAbstractListModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a common practice to create separate models to use only as an index over some display model? My understanding is that we generally don't create a model for something we don't intend to display in the UI. And you're not planning to display all resources as a list, right? If you're just adding this model class as something that listens for resource changes and updates disk, I don't think that's right. I think it can just be a normal manager class. @RobertBColton please confirm or deny.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is displayed tho
if (role != Qt::DisplayRole || orientation != Qt::Orientation::Horizontal) return QVariant(); | ||
|
||
switch(section) { | ||
case 0: return tr("Tree"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know a better way of doing this, but I feel like there probably is one... I mean, naively, you can just put those labels in an array. But I'm not sure what happens if the user tries to drag columns around or something; I feel like Qt probably has better support for this somewhere.
@@ -27,19 +26,35 @@ class MainWindow; | |||
class MainWindow : public QMainWindow { | |||
Q_OBJECT | |||
|
|||
private: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This move confused me for a second. As a style rule, we (primarily Google) put the private block at the bottom of the class (so that things we want people to know about are on top).
@@ -0,0 +1,15 @@ | |||
#ifndef GITTREESTYLEDDELEGATE_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File comment: I'd move the delegates and various Git list facets to a subfolder of this components folder, to improve navigability of the codebase. It's honestly a big feature, which is why I thought it'd make a good intern project.