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

Move various state computations from the UI to the NodeModel #434

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jarolrod
Copy link
Member

@jarolrod jarolrod commented Nov 21, 2024

To enforce clear seperation between the UI and the backend, as well as to begin the removal of any js functions, this shifts the following string construction and state computations from the UI into the NodeModel itself (which is where it belongs)

  • formatted verification progress string
  • connected state
  • synced state
  • estimating state
  • formatted remaining sync time

These strings and states are now available and reusable wherever they may be needed, instead of needing to be computed by every file which needs that information.

As a rule, the UI should have no computation of any sorts outside of things related to UI specific ideas such as color or sizing; any other computation needs to be strictly done in our backend.

Instead of having the UI compute the formatted version of verification
progress using js, move this computation into the nodemodel; where it
belongs.

The UI is the UI, and the backend is the backend.
Instead of having the UI compute when the node is connected to the
network, move this computation into the nodemodel; where it belongs.

The UI is the UI, and the backend is the backend.
Instead of having the UI compute when the node is synced, move this
computation into the nodemodel; where it belongs.

The UI is the UI, and the backend is the backend.
Instead of having the UI construct the formatted version of the
remaining sync time as well as computing when the node is estimating the
remaining sync time using js, move this construction and computation
into the nodemodel; where it belongs.

The UI is the UI, and the backend is the backend.
This is not needed, and we cannot have any javascript functions.
Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

cr ACK

It looks much cleaner. Regarding the formatting, as discussed offline, I'd take it out from the NodeModel, I think it's not its responsibility and not related with business logic and the node itself, I'd create a separate object to make the formatting.

Copy link
Contributor

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

I wanted to expand on my previous review and provide additional details, especially since we discussed in our call that more context and actionable feedback would be helpful.

Why this is important:

  • Separation of concerns: By isolating formatting logic, NodeModel can focus on its primary responsibilities.
  • Reusability: Formatting/conversion logic can be reused across modules or models, especially as we scale and introduce similar needs elsewhere.
  • Improved collaboration: Keeping NodeModel lean ensures that team members working on different aspects of the model don't block each other over formatting corrections or tweaks, enabling smoother parallel development.

Counterpoint: While we were advised to avoid generalizing for single-use cases, I believe this is an exception for two reasons:

  1. Formatting/conversion needs are likely to emerge in multiple places over time.
  2. Even if it's only a single case today, the reduction in potential merge conflicts and enhanced modularity justify the small upfront investment.

Specific Suggestions:

  • Refactor the large setFormattedRemainingSyncTime function into a TimeFormatter class or similar utility.

    Snippet code sample.
    // NodeFormatter.h
    #ifndef NODEFORMATTER_H
    #define NODEFORMATTER_H
    
    #include <QString>
    
    class NodeFormatter {
    public:
        static QString formatRemainingSyncTime(int new_time, bool& estimating);
        static QString formatVerificationProgress(double progress);
    };
    
    #endif // NODEFORMATTER_H

    Modify NodeModel to delegate formatting logic to NodeFormatter:

    #include "NodeFormatter.h"
    
    // Existing methods
    void NodeModel::setFormattedRemainingSyncTime(int new_time)
    {
        bool estimating = false;
        m_formatted_remaining_sync_time = NodeFormatter::formatRemainingSyncTime(new_time, estimating);
        setEstimatingSyncTime(estimating);
    }
    
    void NodeModel::setFormattedVerificationProgress(double new_progress)
    {
        m_formatted_verification_progress = NodeFormatter::formatVerificationProgress(new_progress);
    }
  • Use a handler pattern to simplify the chain of if conditions.

    Snippet code sample.
    class TimeFormatterHandler {
    public:
        virtual ~TimeFormatterHandler() = default;
        virtual bool canHandle(int new_time) const = 0;
        virtual QString handle(int new_time, bool& estimating) const = 0;
    };
    
    // Example: WeeksHandler
    class WeeksHandler : public TimeFormatterHandler {
    public:
        bool canHandle(int new_time) const override {
            return new_time >= 10080 * 60000;
        }
    
        QString handle(int new_time, bool& estimating) const override {
            int weeks = new_time / (10080 * 60000);
            estimating = false;
            return QObject::tr("~%1 %2 left").arg(weeks).arg(weeks == 1 ? QObject::tr("week") : QObject::tr("weeks"));
        }
    };
    
    // Add other handlers: DaysHandler, HoursHandler, etc.
    
    QString NodeFormatter::formatRemainingSyncTime(int new_time, bool& estimating) {
        static const std::vector<std::unique_ptr<TimeFormatterHandler>> handlers = {
            std::make_unique<WeeksHandler>(),
            // Add other handlers in priority order
        };
    
        for (const auto& handler : handlers) {
            if (handler->canHandle(new_time)) {
                return handler->handle(new_time, estimating);
            }
        }
    
       estimating = true;
       return QObject::tr("Estimating");
    }

Comment on lines +101 to +103
setFormattedRemainingSyncTime(m_remaining_sync_time);

Q_EMIT remainingSyncTimeChanged();
Copy link
Member

@hebasto hebasto Dec 16, 2024

Choose a reason for hiding this comment

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

Please remove trailing spaces to fix the CI lint job:

Suggested change
setFormattedRemainingSyncTime(m_remaining_sync_time);
Q_EMIT remainingSyncTimeChanged();
setFormattedRemainingSyncTime(m_remaining_sync_time);
Q_EMIT remainingSyncTimeChanged();

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.

3 participants